From d3ce74395182dc6250eef78125e26583364da0cc Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 19 Aug 2026 07:18:24 +0000 Subject: [PATCH 01/11] Bind mount Actor dev folder for rebuild-free local iteration Register an Actor's host source folder and mount it over the image working directory on every run, so a local `tsc` is enough to pick up TS changes. The folder is registered through a new local-only endpoint outside the emulated Apify /v2 surface, and through a form on the console Actor page. Registration verifies the folder exists host-side via a create-only probe container, since the runtime is itself containerized and cannot stat a host path. Mounts (not Binds) is used throughout so a missing source is an error rather than an auto-created empty directory masking the working directory. Requirements updated to match: dockerode instead of a shelled-out docker inspect, --mount semantics instead of -v, the new endpoint namespace, and the console no longer being strictly view-only. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_011L4VcFqN9UZUbVRMvQSugv --- README.md | 31 ++ requirements/actor-driver.md | 93 ++++- requirements/api.md | 56 ++- requirements/console.md | 34 +- requirements/storage.md | 23 +- src/api/routes/dev-folder.ts | 60 +++ src/api/server.ts | 6 + src/console/server.ts | 81 +++- src/driver/docker-driver.ts | 157 +++++++- src/driver/types.ts | 48 +++ src/index.ts | 2 +- src/services/actors.ts | 166 ++++++++ src/services/builds.ts | 18 +- src/services/runs.ts | 11 + src/storage/entities.ts | 22 ++ test/e2e/dev-folder-bind-mount.test.ts | 245 ++++++++++++ test/integration/console.test.ts | 2 +- test/integration/dev-folder.test.ts | 497 ++++++++++++++++++++++++ test/integration/shutdown.test.ts | 2 +- test/unit/dev-folder-validation.test.ts | 50 +++ test/unit/docker-driver.test.ts | 332 +++++++++++++++- 21 files changed, 1906 insertions(+), 30 deletions(-) create mode 100644 src/api/routes/dev-folder.ts create mode 100644 test/e2e/dev-folder-bind-mount.test.ts create mode 100644 test/integration/dev-folder.test.ts create mode 100644 test/unit/dev-folder-validation.test.ts diff --git a/README.md b/README.md index c90b664..831a726 100644 --- a/README.md +++ b/README.md @@ -40,6 +40,37 @@ the real platform is reachable, the runtime also adopts that account's real user the first time it sees the token; fully offline (or with any other non-empty token) it just keeps using the single local user, with no error either way - see `requirements/cli.md`'s User bootstrap section. +## Rapid dev loop: bind-mounting your local source (no rebuild per edit) + +After the one push+build above, register your Actor's local source folder so every future run picks up +local edits without a rebuild: + +```bash +apify api POST ../actor-runtime/dev-folder/ --body '"/abs/path/to/sample_actor_ts"' +``` + +`` is the id `apify push --json` printed (`.actor.id`); the path must be absolute and must +already exist on the **host** - the runtime verifies this by actually trying to mount it, and rejects +the call with a clear error if the Actor has no successful build yet or the path can't be confirmed. +The same thing is also a single-field form on the Actor's page in the console (`http://localhost:3000`). + +From then on: + +```bash +# edit src/main.ts, then: +npm run build # recompile locally - tsc, no apify push +apify call --input '{"maxPages":3}' # picks up the new dist/, no rebuild +``` + +Node doesn't hot-reload a running process, so a local recompile is picked up by the **next** run's +container start, not by any run already in progress. `node_modules` inside the container still comes +from the built image - an anonymous volume preserves it underneath the bind mount - so a new dependency +in `package.json` still needs a real `apify push`/build; only source edits skip it. Clear the +registration with an empty body (`--body '""'`) to go back to running purely from the built image. Full +mechanics: `requirements/actor-driver.md`'s "Bind mount volumes with Actor source code"; +endpoint/console details: `requirements/api.md`'s `/actor-runtime/*` section and +`requirements/console.md`. + ## Development ```bash diff --git a/requirements/actor-driver.md b/requirements/actor-driver.md index b2d4c23..db1fe5c 100644 --- a/requirements/actor-driver.md +++ b/requirements/actor-driver.md @@ -39,10 +39,95 @@ - Actor details are saved in `__ACTORS__` internal storage # Bind mount volumes with Actor source code -- To enable rapid development of Actors, it is desired to avoid the need to rebuild the Actors. This can be achieved by bind mounting the actor local development folder when starting the container. -- When building an Actor, get the location where the Actor source code is locally located and save it in `__ACTORS__` under `localDevFolder` -- Detect the working directory of docker image `docker inspect -f '{{.Config.WorkingDir}}' {DOCKER_IMAGE}` (replace {DOCKER_IMAGE} by the docker image identifier) and save it to `__ACTORS__` under `imageWorkingDirectory` -- Each Actor stored by the local Actor runtime is started with bind mounted development folder using these additional arguments `-v {localDevFolder}:{imageWorkingDirectory} -v {imageWorkingDirectory}/node_modules` + +- To enable rapid development of Actors, it is desired to avoid the need to rebuild the Actors for + every source change. This is achieved by bind mounting the Actor's local development folder over the + built image's working directory when starting the container - edit locally, recompile locally + (`tsc`, or the language-appropriate equivalent), `apify call` again, with no `apify push`/build in + between. Node does not hot-reload a running process, so the recompiled output is picked up by the + **next run's container start**, not inside an already-running container - this matches the runtime + exactly, since it creates a fresh container per run. +- `localDevFolder` is **registered explicitly**, not learned automatically from a build: a new + local-only endpoint, `POST /actor-runtime/dev-folder/:actorId` (see `api.md`), also exposed as a + single-field form on the console's Actor detail view (`console.md`), sets or clears it. Both surfaces + funnel through one shared validate-and-persist service function. This is a deliberate correction of + this section's original phrasing ("when building an Actor, get the location...") - nothing on the + wire between `apify push` and this runtime ever carries a host filesystem path, so there is no + build-time signal to "get" it from; the developer supplies it out-of-band, once, after their first + successful push+build. +- **Registration requires a prior successful build.** The registration path verifies the candidate + folder against the Actor's own latest successfully-built image (see below), so an Actor with no + successful build at all is rejected with a clear error at registration time, telling the developer to + build first - never a silent accept with nothing to check against. +- **Registration validates the path in two layers**, not shape alone: + 1. A cheap shape pre-filter: the submitted value must be an absolute POSIX path, contain no newline + or NUL byte, and stay under a length cap. A leading `~` is never expanded - this runtime never + shells out to interpret one. + 2. A **host-side existence check**. The runtime process's own filesystem is not the host's (this + runtime always runs containerized itself, talking to the _host_ Docker socket - `fs.existsSync` + here would test the wrong filesystem entirely). The only Docker Engine API surface that validates + an arbitrary host path at all is the mount-validation the daemon runs inside `POST +/containers/create`, so the check is a **create-only probe container, never started**: a container + is created with a single `Mounts` entry (`Type: 'bind'`, the candidate path as `Source`, read-only) + against the Actor's own latest successfully-built image; success removes it immediately without + ever calling `.start()`, and a rejection means creation itself failed, so there is nothing to clean + up either way. + - Submitting the **empty string clears the registration** and never runs either validation layer - + there is no path to check, and clearing must always succeed, including when Docker itself is + unreachable. + - Errors are classified by shape, most specific first, and every non-success branch rejects rather + than guessing: no HTTP response at all (Docker itself unreachable) is reported as "could not verify + - Docker is unreachable", never as "does not exist"; the probe's own image returning 404 is an + operational fault ("could not verify - internal error"), not a bad path; a mount-validation + rejection whose message contains the exact substring `bind source path does not exist` is the one + case reported as "path does not exist"; every other mount-validation-shaped rejection (not a + directory, a permission error, Docker Desktop's file-sharing denial, or anything unrecognized) is + reported as a generic "could not verify this path" - never a false "does not exist". +- **`imageWorkingDirectory` is captured by the driver itself**, right after a successful build: + `docker.getImage(imageId).inspect()` over `dockerode`, reading `.Config.WorkingDir`, then persisted to + `__ACTORS__` in the same write that records the tagged build. This corrects this section's original + shelled-out-CLI phrasing for detecting an image's working directory - this codebase talks to the host + Docker socket exclusively through `dockerode`, never by shelling out to a `docker` command-line + invocation, matching every other Docker interaction in `actor-driver.md`. An inspect failure is logged + and tolerated - it must + never fail an otherwise-successful build - and an empty or `/` working directory is left unset the + same way: mounting a dev folder over `/` would destroy the container. This field reflects the Actor's + _most recent_ successful build; running an older, differently-tagged build whose image had a + different working directory is a known staleness gap, accepted for the POC. +- **The mount is conditional, applied only when both fields are present and non-empty** - + `localDevFolder` and a known, non-`/`, non-empty `imageWorkingDirectory`. An Actor that was never + registered (or was cleared) starts exactly as if this feature did not exist: no mount-related entries + at all in its container's configuration. +- **The mount uses `HostConfig.Mounts`, never the legacy `Binds` array or literal `-v` flags.** This + corrects the section's original `-v {localDevFolder}:{imageWorkingDirectory} -v +{imageWorkingDirectory}/node_modules` phrasing: a plain `-v`/`Binds` bind **auto-creates** a missing + host source directory silently, which would defeat the whole point of validating existence at + registration and would let a folder that vanished between registration and a run start silently mount + an empty directory over the image's working directory instead of failing the run. A `Mounts`-type + bind **errors** on a missing source instead (unless `BindOptions.CreateMountpoint` is explicitly set, + which this runtime never does), giving the same strictness at run start as at registration. One + `HostConfig.Mounts` array carries both entries: + - `{ Type: 'bind', Source: localDevFolder, Target: imageWorkingDirectory }` - read-write (no + `ReadOnly`), matching this section's original plain, unsuffixed mount intent. + - `{ Type: 'volume', Source: '', Target: '{imageWorkingDirectory}/node_modules' }` - the + `Mounts`-array equivalent of the anonymous-volume, bare-container-path `-v` form. Docker copies the + image's existing contents into an anonymous volume before mounting it, which is exactly what + preserves the image's own installed `node_modules` underneath a bind that otherwise covers the + whole working directory: a _named_ volume would start empty, and a plain bind would erase it + entirely. Dependency changes therefore still require a real rebuild - a new package in + `package.json` only lands in the image (and so in the preserved `node_modules`) after one. +- **Every run's container removal passes `{ v: true }`**, on both the normal per-run removal and + startup's orphan reconciliation. Without it, the anonymous `node_modules` volume above would leak one + volume per run, forever, silently filling the host's disk - introducing the anonymous volume without + this fix is not safe. +- **Observability**: since a folder's existence is verified at registration, a folder that is later + deleted, moved, or made unreadable before a run starts is now a residual, not the primary, risk - the + run's log opens with an explicit line naming the host path and the container path being mounted, so a + run that fails against a since-vanished folder explains why in its very first log line, and the + daemon's own `Mounts`-type rejection (see above) fails that run loudly rather than silently mounting + an empty auto-created directory. +- Neither `localDevFolder` nor `imageWorkingDirectory` is ever exposed on the public `/v2` API + (`storage.md`). # Networking diff --git a/requirements/api.md b/requirements/api.md index c3c5acf..8d2900a 100644 --- a/requirements/api.md +++ b/requirements/api.md @@ -124,7 +124,9 @@ implement. Not built here; both paths are in the vendored spec table (`api/spec-table.ts`) as `implemented: false` so they answer `501`, not `404`. - All endpoints from the specification that do not have implementation must return response `501 Not Implemented` -- All endpoints not present in specification must return `404 Not Found` +- All endpoints not present in specification must return `404 Not Found` - **except** the + `/actor-runtime/*` namespace below, which is deliberately outside the Apify spec entirely and is + never routed through the `501`/`404` classification this rule describes. # Private API @@ -133,3 +135,55 @@ ## Upstream fallback (opt-in, off by default, all HTTP methods) - Not implemented + +# `/actor-runtime/*` - a deliberately non-Apify, local-only namespace + +- Everything under `/actor-runtime/*` is this runtime's own tooling surface, not part of the emulated + Apify `/v2` API - it exists only because this runtime runs locally, and has no equivalent on the real + platform. The "every off-spec path returns 404" rule above (and "501 vs 404") applies only to paths + that are, or resemble, real Apify spec paths; `/actor-runtime/*` is carved out of that rule entirely, + as a namespace, not as a one-off exception for a single route. +- It is mounted directly on the API app, outside the `v2` router, and registered before the + `501`/`404` catch-all - so unlike every path under `/v2`, a route here is not classified against the + vendored Apify spec table at all. +- **`POST /actor-runtime/dev-folder/:actorId`** - registers (or clears) the Actor's local dev folder for + the bind-mount feature (`actor-driver.md`'s "Bind mount volumes with Actor source code"). `:actorId` + accepts the same forms as the rest of the API (id, plain name, `username~name`). + - **Authenticated** the same way as every `/v2` route (`Authorization: Bearer ` or `?token=`), + even though it sits outside the `v2` router and so does not inherit that router's `auth()` + middleware automatically - it applies its own. Ownership-scoped via the same `resolveOwnedActor` + lookup `/v2` uses: a caller can only register a dev folder for their own Actor, and a mismatched + or nonexistent `:actorId` answers `404` with error type `record-not-found`, exactly like the rest + of the API. + - **Request body**: a JSON string - `'"/abs/path/to/src"'` to set, `'""'` to clear, trimmed after + parsing. This is deliberate, not merely convenient: `apify api`'s `--body` flag validates with + `JSON.parse` and refuses anything that is not valid JSON, so a bare, unquoted path can never reach + this route through the documented CLI invocation at all. A body that is not valid JSON, or is + valid JSON but not a string (a bare number, an object, ...), is rejected with `400` / + `invalid-request` - never silently coerced or stored verbatim. + - **Response**: on success, `{ data: { localDevFolder, imageWorkingDirectory, mountWillApply } }` - + the same three values the console detail page shows (`console.md`), doubling as the read-back this + design has no separate `GET` for. + - **Error responses**, by rejection reason (`actor-driver.md` has the full validation/classification + detail): + - `400` `invalid-request` - the body isn't a JSON string, or the string fails the absolute-path + shape check. + - `400` `dev-folder-not-buildable` - the Actor has never had a successful build. + - `400` `dev-folder-path-not-found` - the host-side probe's daemon rejection contained the exact + "bind source path does not exist" substring. + - `400` `dev-folder-check-failed` - any other mount-validation-shaped rejection; reported as + "could not verify", never as "does not exist". + - `503` `dev-folder-check-unavailable` - Docker itself is unreachable. + - `500` `internal-error` - the probe's own image is missing (a 404 from the daemon on an image + that should exist) - an operational fault, not a bad submitted path. + - These exact codes/types are an implementation choice, not a spec-level commitment the way the + `/v2` envelope contract is - but the _distinctions themselves_ (does-not-exist vs. could-not-verify, + build-first vs. bad-path) are load-bearing and must not collapse into one generic error. + - The documented CLI invocation is + `apify api POST ../actor-runtime/dev-folder/ --body '"/abs/path/to/src"'` - the CLI's + `apify api` escape hatch, whose `../` resolves past its own configured `/v2`-suffixed base URL and + lands on this route directly, carrying the same bearer token every other `apify` command sends. +- The console's own dev-folder form (`console.md`) does **not** go through this endpoint - it posts to a + console-local, unauthenticated route on the console's own port, resolving the Actor cross-user by the + id already in its page URL. Both routes funnel into the same underlying validate-and-persist service + function, so the two surfaces can never drift apart in behavior, only in how they are reached. diff --git a/requirements/console.md b/requirements/console.md index b36c056..1cb6731 100644 --- a/requirements/console.md +++ b/requirements/console.md @@ -1,14 +1,22 @@ # Frontend -- Console frontend is simple view-only page that allows to inspect each user object. +- Console frontend is a page that allows inspecting each user's objects across the whole runtime. - Server-rendered HTML from the same process that serves the API, on its own fixed port (3000). No SPA, no bundler, no build step - plain Express routes returning HTML strings. It reads through the same service layer as the API handlers, so storage/build/run access logic is shared rather than reimplemented. - Frontend shows for each object the owner (`userId`). -- The console has no login of its own (it is unauthenticated and view-only), so with multiple users it - lists and shows every user's objects rather than scoping to one - the API's own endpoints stay - strictly scoped to the calling token's user (`storage.md`'s "Users" section). +- The console has no login of its own, so with multiple users it lists and shows every user's objects + rather than scoping to one - the API's own endpoints stay strictly scoped to the calling token's user + (`storage.md`'s "Users" section). +- **The console is unauthenticated, and view-only except for exactly one mutation**: the Actor detail + view's local dev-folder registration form (below). This amends the previous blanket "unauthenticated + and view-only" statement, which no longer describes the shipped console accurately - it still has no + login of its own, and every other route remains a plain read, but that one form genuinely writes + state. It writes cross-user the same way the console's reads already are cross-user: resolving the + Actor by the id already in the page URL, with no token and no ownership check - a documented deviation + from the API's own strictly owner-scoped equivalent write, not an accident (`actor-driver.md`, + `api.md`'s `/actor-runtime/*` section). - There are three types of objects: key-value store, dataset, request queue. - For each object type there must be exactly one widget for inspection. - The request-queue widget leads with the authoritative counts from `RequestQueue.getInfo()` @@ -34,3 +42,21 @@ list's dataset column), not as plain text. - Log views render ANSI colors from actor output as HTML, while the `/v2/logs/:id` API keeps serving logs raw (unconverted) for the CLI to render itself. - The console accepts the real Apify Console's URL shapes (as printed by stock apify-cli, e.g. `/actors/:actorId/runs/:runId`, `/storage/datasets/:id`) via redirects to its own pages. + +## Local dev-folder registration form (Actor detail view) + +- The Actor detail view shows three additional, always-visible values, reflecting the same underlying + state the API's `POST /actor-runtime/dev-folder/:actorId` reads and writes (`api.md`): the currently + registered local dev folder (or a clear "none registered" state), the detected image working + directory (or a clear "not yet detected" state when no build has produced one yet), and an explicit + indication of whether a mount will therefore be applied on the Actor's next run. +- A single-field form submits to a console-local `POST /actors/:id/dev-folder` route (urlencoded, not + JSON) - the console's own port, not the API's - resolving the Actor the same cross-user way every + other console read does. It funnels into the same validate-and-persist service function the API + endpoint uses, so the two surfaces can never observe or produce different outcomes for the same input. + Submitting an empty value clears the registration, exactly like the API's empty-JSON-string body. +- A submission that fails validation (a relative path, an Actor with no successful build, a path the + host-side probe could not confirm exists, Docker being unreachable, ...) redirects back to the same + detail page with the classified error message shown inline - the build-first rejection and the + does-not-exist/could-not-verify distinction are surfaced to whoever is looking at the page, never + swallowed by the redirect. diff --git a/requirements/storage.md b/requirements/storage.md index 0459a4b..a68b1c6 100644 --- a/requirements/storage.md +++ b/requirements/storage.md @@ -71,8 +71,27 @@ - `value` is the metadata of the Actor - owner (`userId`) - metadata - - localDevFolder - - imageWorkingDirectory + - `localDevFolder` - **optional**. Absent means no dev folder has ever been registered for this + Actor. Set/cleared only through `POST /actor-runtime/dev-folder/:actorId` or the console's + equivalent form (`api.md`, `console.md`) - never as a side effect of any other Actor write + (pushing a version, starting a build, etc.). Submitting the empty string is a distinct, + first-class "clear" operation, not a stored empty-string value: it removes the field entirely, + the same as if it had never been registered - there is no state that distinguishes "cleared" + from "never set". When present, it is always a non-empty absolute host path that passed both + the shape check and the host-side existence probe at the time it was registered + (`actor-driver.md`). + - `imageWorkingDirectory` - **optional**. Absent until at least one build has succeeded and its + image's working directory could be captured (`.Config.WorkingDir` via `dockerode`, never by + shelling out to a `docker` command-line invocation); also absent when the captured value was + empty or `/`. Written by + the driver in the same `updateActor` call that records a successful build's `taggedBuilds` + entry, and reflects only the _most recent_ successful build. + - Both fields are **absent (or empty) meaning no mount**: `startRun` only adds the dev-folder bind + mount when both are present and non-empty simultaneously (`actor-driver.md`) - an Actor missing + either one starts exactly as if the feature did not exist. + - Neither field is ever exposed on the public `/v2` API - `api/dto/actors.ts`'s `actorDto` is + explicit field-by-field, so a new `ActorRecord` field cannot leak into a `/v2` response by + construction. - The system stores Actor runs in dedicated key-value store called `__RUNS__`: - `key` is the id of the Actor run `runId` - `value` is the metadata of the Actor diff --git a/src/api/routes/dev-folder.ts b/src/api/routes/dev-folder.ts new file mode 100644 index 0000000..1dad145 --- /dev/null +++ b/src/api/routes/dev-folder.ts @@ -0,0 +1,60 @@ +/** + * `POST /actor-runtime/dev-folder/:actorId` - deliberately outside the emulated `/v2` surface + * (`api.md`'s `/actor-runtime/*` namespace; `design.md`'s Decisions #1/#8), so this is mounted directly + * on the API `app`, not the `v2` router (`server.ts`'s "Auth is per-router, not global" note) - it + * therefore needs its own `auth()`, applied to a small router of its own below, not inherited from `v2`. + * + * Canonical body is a JSON string: `'"/abs/path"'` to set, `'""'` to clear (`design.md`'s Decision #6 - + * `apify api`'s own `--body` validates with `JSON.parse` and refuses anything that isn't valid JSON, so + * a bare, unquoted path can never reach this route through the documented CLI invocation at all). A + * JSON value that parses but isn't a string (a number, an object, ...) is rejected the same way a + * malformed body is - only a genuine JSON string is ever a valid registration payload. + * + * Ownership-scoped like every other Actor write on this API port: `resolveOwnedActor` (not the + * console's cross-user `getActorById`) so a caller can only ever register a dev folder for their own + * Actor, and can name it by id, plain name, or `username~name` the same way `POST .../builds` and + * `POST .../runs` already do. + */ +import express, { type Express } from 'express'; + +import { auth, requireUser } from '../auth.js'; +import { sendData } from '../envelope.js'; +import { ApiError, recordNotFound } from '../errors.js'; +import { h, jsonBody } from '../handler.js'; +import { describeDevFolderError, devFolderStatus, resolveOwnedActor, setDevFolder } from '../../services/actors.js'; +import type { ApiServerDeps } from '../server.js'; + +export function mountDevFolder(app: Express, deps: ApiServerDeps): void { + const router = express.Router(); + router.use(auth()); + + router.post( + '/dev-folder/:actorId', + h(async (req, res) => { + const user = requireUser(req); + const actor = await resolveOwnedActor(user.id, req.params.actorId as string, user.username); + if (!actor) throw recordNotFound(); + + const raw = jsonBody(req); + if (typeof raw !== 'string') { + throw new ApiError( + 400, + 'invalid-request', + 'Request body must be a JSON string - e.g. "/abs/path/to/src" to set, or "" to clear', + ); + } + + const result = await setDevFolder(deps.driver, actor, raw.trim()); + if (result.kind !== 'ok') { + const info = describeDevFolderError(result); + throw new ApiError(info.status, info.type, info.message); + } + + // The response body doubles as the read-back this design deliberately has no separate `GET` + // for yet (`design.md`'s Follow-ups) - the same three fields the console detail page shows. + sendData(res, devFolderStatus(result.actor)); + }), + ); + + app.use('/actor-runtime', router); +} diff --git a/src/api/server.ts b/src/api/server.ts index 03b6e60..5c51af2 100644 --- a/src/api/server.ts +++ b/src/api/server.ts @@ -13,6 +13,7 @@ import { mountBuilds } from './routes/builds.js'; import { mountRuns } from './routes/runs.js'; import { mountLogs } from './routes/logs.js'; import { mountRunStorageAliases } from './routes/run-storage-aliases.js'; +import { mountDevFolder } from './routes/dev-folder.js'; import type { Driver } from '../driver/types.js'; export interface ApiServerDeps { @@ -42,6 +43,11 @@ export function createApiServer(deps: ApiServerDeps): Express { app.use('/v2', v2); + // `/actor-runtime/*` - a deliberately non-Apify, local-runtime-only namespace (`api.md`), registered + // before the 501/404 catch-all below but outside the `v2` router entirely, so it needs its own + // `auth()` (see `mountDevFolder`'s doc comment) rather than inheriting `v2.use(auth())` above. + mountDevFolder(app, deps); + app.use((req: Request, res: Response) => { const path = req.path.replace(/^\/+/, ''); const entry = matchSpecPath(req.method, path); diff --git a/src/console/server.ts b/src/console/server.ts index 45db364..55199b9 100644 --- a/src/console/server.ts +++ b/src/console/server.ts @@ -4,16 +4,24 @@ * through the same service layer as the API handlers, so ownership filtering (over on the API side) is * shared rather than reimplemented. * - * The console itself has no login of its own - it is a view-only, unauthenticated local dev tool - * (`console.md`) - so with multiple users it does not scope to any one of them: every list/detail route + * The console itself has no login of its own - it is unauthenticated, and view-only except for exactly + * one mutation (`console.md`, amended by `design.md`'s Decision #3): the dev-folder form on the Actor + * detail view. With multiple users it does not scope reads to any one of them: every list/detail route * below reads through the `listAll*`/`get*ById` cross-user service functions (see e.g. * `services/actors.ts: listAllActors`), never the API's own per-user `listOwned*`/`getOwned*`, and every * list row and detail view shows the object's owner `userId` (`console.md`: "Frontend shows for each - * object the owner (userId)"). + * object the owner (userId)"). The dev-folder form writes cross-user the same way - a documented + * deviation from the API's own strictly-owner-scoped write, not an accident (`design.md`'s Risks). */ import express, { type Express } from 'express'; -import { getActorById, listAllActors } from '../services/actors.js'; +import { + describeDevFolderError, + devFolderStatus, + getActorById, + listAllActors, + setDevFolder, +} from '../services/actors.js'; import { getBuildById, listAllBuilds } from '../services/builds.js'; import { getRunById, listAllRuns } from '../services/runs.js'; import { getFullLog } from '../services/logs.js'; @@ -24,15 +32,47 @@ import { pageKeys } from '../services/kv-key-listing.js'; import { applyDatasetProjection, type DatasetItem } from '../services/dataset-projection.js'; import { ansiToHtml } from './ansi.js'; import { definitionList, escapeHtml, layout, table, type LinkedCell } from './templates.js'; +import type { Driver } from '../driver/types.js'; /** A run's default-storage id rendered as a link to that storage's detail view instead of plain text. */ function storageLink(prefix: '/datasets' | '/key-value-stores' | '/request-queues', id: string): LinkedCell { return { text: id, href: `${prefix}/${encodeURIComponent(id)}` }; } -export function createConsoleServer(): Express { +export interface ConsoleServerDeps { + driver: Driver; +} + +/** The dev-folder registration form + its three read-only status rows, rendered on the Actor detail + * view (`design.md`'s console decisions). `errorMessage` is threaded through from the POST handler's + * redirect query param below, since a redirect itself carries no state of its own. */ +function devFolderSection(actorId: string, status: ReturnType, errorMessage?: string): string { + const errorHtml = errorMessage + ? `

Error: ${escapeHtml(errorMessage)}

` + : ''; + return ( + '

Local dev folder

' + + definitionList([ + ['localDevFolder', status.localDevFolder ?? '(none registered)'], + ['imageWorkingDirectory', status.imageWorkingDirectory ?? '(not yet detected - build the Actor first)'], + ['mount will apply on the next run', String(status.mountWillApply)], + ]) + + errorHtml + + `
` + + ` ' + + '' + + '
' + + '

Submit an empty value to clear the registration.

' + ); +} + +export function createConsoleServer(deps: ConsoleServerDeps): Express { const app = express(); app.disable('x-powered-by'); + // Only the dev-folder form below posts anything - every other console route is a plain `GET` + // (`console.md`'s "view-only except for exactly one mutation"). + app.use(express.urlencoded({ extended: false })); app.get('/', async (_req, res) => { res.send(layout('actor-runtime', '

Pick an object type from the navigation above.

')); @@ -59,6 +99,7 @@ export function createConsoleServer(): Express { res.status(404).send(layout('Not found', '

Actor not found.

')); return; } + const devFolderError = typeof req.query.devFolderError === 'string' ? req.query.devFolderError : undefined; const body = definitionList([ ['id', actor.id], @@ -79,10 +120,38 @@ export function createConsoleServer(): Express { Object.entries(actor.taggedBuilds).map(([tag, b]) => [tag, b.buildId, b.buildNumber]), 1, '/builds', - ); + ) + + devFolderSection(actor.id, devFolderStatus(actor), devFolderError); res.send(layout(`Actor ${actor.name}`, body)); }); + /** + * The console's one mutation (`design.md`'s Decision #3) - funnels through the exact same + * `setDevFolder` the API's `POST /actor-runtime/dev-folder/:actorId` uses, resolving the Actor + * cross-user by the id already in the page URL (no token, matching the console's existing + * unauthenticated reads) rather than through `resolveOwnedActor`. A failure redirects back with the + * classified message in a query param - `describeDevFolderError`'s wording, not a bespoke one - so + * the build-first rejection and the does-not-exist/could-not-verify distinction are surfaced, not + * swallowed (success criterion 27). + */ + app.post('/actors/:id/dev-folder', async (req, res) => { + const actor = await getActorById(req.params.id); + if (!actor) { + res.status(404).send(layout('Not found', '

Actor not found.

')); + return; + } + const body = req.body as Record | undefined; + const submitted = typeof body?.localDevFolder === 'string' ? body.localDevFolder.trim() : ''; + + const result = await setDevFolder(deps.driver, actor, submitted); + if (result.kind !== 'ok') { + const info = describeDevFolderError(result); + res.redirect(`/actors/${encodeURIComponent(actor.id)}?devFolderError=${encodeURIComponent(info.message)}`); + return; + } + res.redirect(`/actors/${encodeURIComponent(actor.id)}`); + }); + // --- Compatibility redirects: stock apify-cli only knows one Console, so it prints links using // the real Apify Console's URL shapes (`/actors/:actorId/runs/:runId`, // `/actors/:actorId/builds/:buildNumber`, `/storage/datasets/:id`, ...). This console serves its own diff --git a/src/driver/docker-driver.ts b/src/driver/docker-driver.ts index f005779..4e9cf5d 100644 --- a/src/driver/docker-driver.ts +++ b/src/driver/docker-driver.ts @@ -1,7 +1,9 @@ /** * The Docker driver: build/run Actor images over the host Docker socket via `dockerode`. Every Actor * container joins the `apify-local` network so it can resolve the runtime's own container by the - * fixed DNS alias `apify-api` - no storage bind mount, HTTP-only storage access (`actor-driver.md`). + * fixed DNS alias `apify-api` (`actor-driver.md`). Storage access is HTTP-only; the one filesystem + * bind mount this driver ever adds is the optional local-dev-folder mount (`RunContext.devMount`, see + * `startRun` below) - conditional, never unconditional, on every Actor's container. * * Genuine cancellation: `docker.buildImage()` accepts an `abortSignal` option that dockerode forwards * all the way to Node's `http.request({ signal })` (`docker-modem/lib/modem.js`: `optionsf.signal = @@ -26,6 +28,9 @@ import { DriverTimedOutError, type BuildContext, type BuildOutcome, + type DevFolderMount, + type DevFolderProbeFailureReason, + type DevFolderProbeOutcome, type Driver, type RunContext, type RunOutcome, @@ -33,6 +38,40 @@ import { const NETWORK_NAME = 'apify-local'; const RUN_LABEL = 'actor-runtime.runId'; +/** Target path for the create-only, never-started existence probe container (`probeDevFolder` below) - + * arbitrary, since the probe is never started and nothing ever reads from it; moby validates the mount + * source before the container object is even returned (`_request_fact_check.md`'s round-3 delta, + * claim 2). */ +const PROBE_MOUNT_TARGET = '/probe'; +/** The daemon's own fixed error-message substring for a `Mounts`-type bind whose source is missing + * (moby's `daemon/volume/mounts/validate.go: errBindSourceDoesNotExist`, pinned by + * `_request_fact_check.md`'s round-3 delta, claim 1) - the one rejection shape `classifyProbeError` + * reports as "does not exist" rather than a generic "could not verify". */ +const BIND_SOURCE_MISSING_SUBSTRING = 'bind source path does not exist'; + +/** Narrows an unknown rejection to the shape `docker-modem` attaches to a daemon HTTP-level error + * response (`Modem.prototype.buildPayload`: `msg.statusCode = res.statusCode`) - present only when the + * daemon actually answered; a raw transport failure (socket refused/missing) carries no `statusCode` at + * all, which is exactly the distinction `classifyProbeError`'s first branch depends on. */ +function hasStatusCode(error: unknown): error is Error & { statusCode: number } { + return ( + typeof error === 'object' && + error !== null && + 'statusCode' in error && + typeof (error as { statusCode: unknown }).statusCode === 'number' + ); +} + +/** Classifies a `createContainer` rejection from `probeDevFolder`, most specific first - see + * `DevFolderProbeFailureReason`'s doc comment in `driver/types.ts` for what each outcome means and why + * a permission error/"not a directory"/Docker Desktop file-sharing denial must never be asserted as + * "does not exist". */ +function classifyProbeError(error: unknown): DevFolderProbeFailureReason { + if (!hasStatusCode(error)) return 'unreachable'; + if (error.statusCode === 404) return 'image-missing'; + if (error.message.includes(BIND_SOURCE_MISSING_SUBSTRING)) return 'not-found'; + return 'unknown'; +} function sourceFileToBuffer(file: SourceFile): Buffer { return file.format === 'BASE64' ? Buffer.from(file.content, 'base64') : Buffer.from(file.content, 'utf8'); @@ -156,7 +195,12 @@ export class DockerDriver implements Driver { return new Promise((resolve, reject) => { this.docker.modem.followProgress( stream, - (err: Error | null, res: Array<{ stream?: string; error?: string; aux?: { ID?: string } }>) => { + // Async: fine even though `followProgress`'s own callback type doesn't expect a Promise back + // (this codebase's `dockerode` types leave `modem` as `any` - see the class doc comment - so + // nothing type-checks the return value either way) - `followProgress` never awaits this + // callback's result, it just invokes it once, and `resolve`/`reject` below settle the outer + // Promise whenever this async function actually gets there. + async (err: Error | null, res: Array<{ stream?: string; error?: string; aux?: { ID?: string } }>) => { cleanup(); if (this.timedOutBuilds.delete(ctx.buildId)) { reject(new DriverTimedOutError(`Build exceeded its ${ctx.timeoutSecs}s timeout`)); @@ -171,7 +215,8 @@ export class DockerDriver implements Driver { reject(new Error(errorLine.error)); return; } - resolve({ imageId: imageTag }); + const imageWorkingDirectory = await this.inspectWorkingDirectory(imageTag); + resolve({ imageId: imageTag, imageWorkingDirectory }); }, (event: { stream?: string; status?: string; error?: string }) => { if (event.stream) onLog(event.stream); @@ -182,6 +227,25 @@ export class DockerDriver implements Driver { }); } + /** + * `.Config.WorkingDir` of the image just built, via `docker.getImage(imageId).inspect()` + * (`design.md`'s "Capture of `imageWorkingDirectory`" - this codebase talks to the host socket + * through `dockerode` only, never a shelled-out `docker inspect`, correcting + * `actor-driver.md`'s original CLI-flag phrasing). An inspect failure is logged and tolerated - it + * must never fail an otherwise-successful build - and an empty or `/` working directory is treated + * the same as "unknown": mounting a dev folder over `/` at run start would destroy the container. + */ + private async inspectWorkingDirectory(imageId: string): Promise { + try { + const info = await this.docker.getImage(imageId).inspect(); + const workingDir = info.Config.WorkingDir; + return workingDir && workingDir !== '/' ? workingDir : undefined; + } catch (error) { + console.warn(`Could not inspect image ${imageId} for its working directory: ${(error as Error).message}`); + return undefined; + } + } + /** * Genuinely interrupts the in-flight build: aborts the `AbortController` passed to `buildImage` as * `abortSignal`, which destroys the underlying HTTP request to the Docker daemon (see the class @@ -198,6 +262,19 @@ export class DockerDriver implements Driver { } const env = Object.entries(ctx.env).map(([key, value]) => `${key}=${value}`); + + // Observability of the mount (`design.md`): a secondary diagnostic now that existence is verified + // at registration - if the folder is deleted/moved/made unreadable between registration and this + // run, the daemon's own rejection below explains why the run failed, but only if the very first + // log line already named the two paths. Written before `createContainer` so it is genuinely first, + // even if the daemon call itself is what ends up failing. + if (ctx.devMount) { + onLog( + `Mounting local dev folder ${ctx.devMount.localDevFolder} over the image's working directory ` + + `${ctx.devMount.imageWorkingDirectory} (node_modules preserved via an anonymous volume).\n`, + ); + } + const container = await this.docker.createContainer({ Image: ctx.imageId, Env: env, @@ -206,6 +283,7 @@ export class DockerDriver implements Driver { NetworkMode: NETWORK_NAME, Memory: ctx.memoryMbytes * 1024 * 1024, AutoRemove: false, + ...(ctx.devMount ? { Mounts: this.buildDevMounts(ctx.devMount) } : {}), }, Tty: false, }); @@ -288,7 +366,75 @@ export class DockerDriver implements Driver { clearTimeout(timeout); this.timedOutRuns.delete(ctx.runId); this.runContainers.delete(ctx.runId); + // `{ v: true }` also removes the container's anonymous volumes - without it, the anonymous + // `node_modules` volume `buildDevMounts` adds for a `devMount` run would leak one volume per run, + // forever (`design.md`'s "Volume cleanup is in this PR"). Harmless for a run with no `devMount`: + // such a container has no anonymous volumes to remove in the first place. + await container.remove({ v: true }).catch(() => undefined); + } + } + + /** + * The two `HostConfig.Mounts` entries for a `devMount` run (`design.md`'s "Applying the mount") - + * `Mounts`, not `Binds`, for the same reason `probeDevFolder` uses `Mounts`: a `Mounts`-type bind + * errors on a missing source instead of silently auto-creating one (`_request_fact_check.md`'s + * round-3 delta, claim 1), so a folder that vanished between registration and this run start fails + * the run loudly instead of masking the image's own working directory with an empty auto-created + * directory. The bind is read-write (no `ReadOnly`), matching the requirement's own plain `-v` form. + * The second entry - `Type: 'volume'` with an empty `Source` - is the `Mounts`-array equivalent of the + * anonymous-volume bare-path `-v` form: Docker copies the image's existing `node_modules` into it + * before mounting, which is what preserves the image's installed dependencies underneath a bind that + * otherwise covers the whole working directory (a *named* volume would start empty; a plain bind + * would erase - `_request_fact_check.md`'s claim 6). + */ + private buildDevMounts(devMount: DevFolderMount): Docker.MountSettings[] { + return [ + { Type: 'bind', Source: devMount.localDevFolder, Target: devMount.imageWorkingDirectory }, + { Type: 'volume', Source: '', Target: `${devMount.imageWorkingDirectory}/node_modules` }, + ]; + } + + /** + * Host-side existence check for a candidate dev-folder path (`design.md`'s "Registration"): a + * create-only probe container, never started. `fs.existsSync` would test this *runtime process's* + * filesystem, not the host's (this driver always runs against the host's own Docker socket - see the + * class doc comment); the only Engine API surface that validates an arbitrary host path at all is the + * mount-validation moby runs inside `POST /containers/create` (`_request_fact_check.md`'s round-3 + * delta, claims 2 and 4). `BindOptions.CreateMountpoint` (the option that would auto-create a missing + * source and defeat this check entirely) is deliberately never set - `@types/dockerode`'s own + * `BindOptions` type doesn't even declare it, so the straightforward, type-safe object literal below + * omits it for free. On success the probe is removed immediately without ever being started; on + * rejection there is nothing to clean up, since creation itself is what failed. + * + * `imageId` is always the Actor's own latest successfully-built image (resolved by + * `services/actors.ts: setDevFolder`), never a self-inspected runtime image or a pulled one - see + * `design.md`'s rejected alternative on self-inspection via `HOSTNAME`, which `selfAttachToNetwork` + * above already documents as unset in bare local dev, exactly where this feature is used. + */ + async probeDevFolder(candidatePath: string, imageId: string): Promise { + // Known-unavailable short-circuits without ever touching the socket - the same outcome + // (`unreachable`) a live daemon that dies mid-call would also produce via `classifyProbeError`'s + // no-`.statusCode` branch, just reached proactively instead of reactively. + if (!this.available) return { ok: false, reason: 'unreachable' }; + + try { + const container = await this.docker.createContainer({ + Image: imageId, + HostConfig: { + Mounts: [ + { + Type: 'bind', + Source: candidatePath, + Target: PROBE_MOUNT_TARGET, + ReadOnly: true, + }, + ], + }, + }); await container.remove().catch(() => undefined); + return { ok: true }; + } catch (error) { + return { ok: false, reason: classifyProbeError(error) }; } } @@ -330,7 +476,10 @@ export class DockerDriver implements Driver { for (const info of containers) { if (!runIdSet.has(info.Labels?.[RUN_LABEL] ?? '')) continue; const container = this.docker.getContainer(info.Id); - await container.remove({ force: true }).catch(() => undefined); + // `{ v: true }` alongside `force: true` - see `startRun`'s finally block's identical fix for why: + // an orphaned run's anonymous `node_modules` volume (if it had a `devMount`) must not survive + // past a restart's reconciliation either. + await container.remove({ force: true, v: true }).catch(() => undefined); } } } diff --git a/src/driver/types.ts b/src/driver/types.ts index 68ecdd4..45d6915 100644 --- a/src/driver/types.ts +++ b/src/driver/types.ts @@ -8,18 +8,53 @@ export interface BuildContext { timeoutSecs: number; } +/** + * Host folder + image working directory, carried together so "both or neither" is enforced by the + * type itself (`design.md`'s "Applying the mount") - there is no way to construct a `RunContext` with + * one field set and the other missing. `services/runs.ts` builds this only when the Actor's own + * `localDevFolder`/`imageWorkingDirectory` are both present and non-empty; `docker-driver.ts`'s + * `startRun` adds the `HostConfig.Mounts` entries only when this is present at all. + */ +export interface DevFolderMount { + localDevFolder: string; + imageWorkingDirectory: string; +} + export interface RunContext { runId: string; imageId: string; env: Record; memoryMbytes: number; timeoutSecs: number; + devMount?: DevFolderMount; } export interface BuildOutcome { imageId: string; + /** `.Config.WorkingDir` of the image `startBuild` just built, captured via + * `docker.getImage(imageId).inspect()` (`design.md`: this codebase talks to the host socket through + * dockerode only, never a shelled-out `docker inspect`). Unset when the inspect call itself failed + * (logged, never fails the build) or when the working directory was empty/`/` (mounting over `/` + * would destroy the container). */ + imageWorkingDirectory?: string; } +/** + * Why a candidate dev-folder path was rejected by the host-side existence probe (`design.md`'s + * "Registration" section), classified by error shape, most specific first: + * - `unreachable`: no HTTP response at all (raw socket error, or the driver already knows Docker is + * unavailable) - never asserted as "does not exist". + * - `image-missing`: the probe's own image (the Actor's latest successfully-built image) returned 404 + * - an operational fault, not a bad path. + * - `not-found`: the daemon's mount-validation rejection message contained the exact substring + * `"bind source path does not exist"` - the one case allowed to say so. + * - `unknown`: any other mount-validation-shaped rejection (not a directory, a Docker Desktop + * file-sharing denial, a permission error, ...) - reported as "could not verify", never as missing. + */ +export type DevFolderProbeFailureReason = 'unreachable' | 'image-missing' | 'not-found' | 'unknown'; + +export type DevFolderProbeOutcome = { ok: true } | { ok: false; reason: DevFolderProbeFailureReason }; + export interface RunOutcome { exitCode: number; /** @@ -65,4 +100,17 @@ export interface Driver { * records have no container of their own to reconcile (see `DockerDriver.reconcileOrphans`'s doc * comment) - orphaned build *records* are still marked `ABORTED` by the caller regardless. */ reconcileOrphans(runIds: string[]): Promise; + + /** + * Host-side existence probe for a candidate dev-folder path (`design.md`'s "Registration"), used + * only by `services/actors.ts: setDevFolder` - never by the build/run lifecycle. Deliberately + * **optional**: every pre-existing stub `Driver` throughout the test suite (none of which model a + * real dockerode handle - `test/integration/helpers/test-server.ts` and several integration test + * files construct `Driver` literals directly) keeps compiling unchanged, since only `DockerDriver` + * and drivers built specifically to exercise dev-folder registration need to implement it. A driver + * that doesn't implement this is treated by `setDevFolder` as unable to verify the path (the + * `unreachable` outcome), which is an accurate description of every such stub - none of them talk to + * a real daemon. + */ + probeDevFolder?(candidatePath: string, imageId: string): Promise; } diff --git a/src/index.ts b/src/index.ts index fe866b3..42002e3 100644 --- a/src/index.ts +++ b/src/index.ts @@ -20,7 +20,7 @@ async function main(): Promise { await reconcileOrphanedJobs(driver); const apiApp = createApiServer({ driver }); - const consoleApp = createConsoleServer(); + const consoleApp = createConsoleServer({ driver }); const apiServer = apiApp.listen(API_PORT); const consoleServer = consoleApp.listen(CONSOLE_PORT); diff --git a/src/services/actors.ts b/src/services/actors.ts index eda45e9..2591c05 100644 --- a/src/services/actors.ts +++ b/src/services/actors.ts @@ -1,6 +1,7 @@ import { generateId } from '../storage/ids.js'; import type { ActorRecord, ActorVersionRecord } from '../storage/entities.js'; import { getRegistries } from '../storage/registries.js'; +import type { Driver, DevFolderProbeOutcome } from '../driver/types.js'; export interface CreateActorInput { name: string; @@ -106,3 +107,168 @@ export function findVersion(actor: ActorRecord, versionNumber: string): ActorVer export function recordTaggedBuild(actor: ActorRecord, tag: string, buildId: string, buildNumber: string): ActorRecord { return { ...actor, taggedBuilds: { ...actor.taggedBuilds, [tag]: { buildId, buildNumber } } }; } + +// --- Local dev-folder registration (`design.md`) --- +// +// One validate-and-persist entry point, `setDevFolder`, shared by the API's +// `POST /actor-runtime/dev-folder/:actorId` (`api/routes/dev-folder.ts`) and the console's single-field +// form (`console/server.ts`) - neither route talks to the registry or the driver's probe directly. Both +// callers pass an already-unwrapped, already-trimmed string: the API unwraps its JSON-string body, the +// console reads its urlencoded form field. + +/** Cheap shape pre-filter, run before the host-side existence check, never instead of it (`design.md`'s + * "Validation is now two layered checks, not shape alone"). `~` is not expanded - this codebase never + * shells out (see `docker-driver.ts`'s class doc comment), so there is no shell to expand it, and + * expanding it here would require guessing which host user's home directory this runtime process + * should assume. Returns `null` for a shape-valid non-empty path, or a human-readable rejection reason. + * Exported for direct unit testing as a pure function; the empty-string "clear" case is handled by + * `setDevFolder` before this is ever called, not inside it. */ +const MAX_DEV_FOLDER_PATH_LENGTH = 4096; + +export function validateDevFolderPathShape(path: string): string | null { + if (path.length > MAX_DEV_FOLDER_PATH_LENGTH) { + return `Path is too long (max ${MAX_DEV_FOLDER_PATH_LENGTH} characters)`; + } + if (!path.startsWith('/')) { + return 'Path must be an absolute POSIX path (starting with "/")'; + } + if (path.includes('\n') || path.includes('\r') || path.includes('\0')) { + return 'Path must not contain a newline or a NUL byte'; + } + return null; +} + +/** + * Resolves the image id `setDevFolder` hands to `driver.probeDevFolder` - the Actor's own latest + * successfully-built image, "the same id the driver already uses to start real runs" (`design.md`). + * `taggedBuilds` is only ever populated by `recordTaggedBuild`, itself only called after a build + * transitions to `SUCCEEDED` (`services/builds.ts: runBuildInBackground`), so any entry at all is proof + * of a genuine past success - matching how `POST /actors/:actorId/runs` resolves a run's build by tag + * (`api/routes/actors.ts`'s `DEFAULT_TAG`), this prefers the `latest` tag when present and otherwise + * falls back to whichever tag exists. Returns `null` when the Actor has never had a successful build at + * all - the build-first precondition (`design.md`'s Decisions #9-adjacent scope-split; success + * criterion 6). + */ +async function resolveProbeImageId(actor: ActorRecord): Promise { + const tags = Object.keys(actor.taggedBuilds); + const preferredTag = actor.taggedBuilds.latest ? 'latest' : tags[0]; + if (!preferredTag) return null; + const tagged = actor.taggedBuilds[preferredTag]; + if (!tagged) return null; + const build = await getRegistries().builds.get(tagged.buildId); + return build?.imageId ?? null; +} + +/** Every way `setDevFolder` can end, `ok` included - a discriminated union so both the API route and the + * console form can map each failure to their own presentation (a JSON error envelope vs. an inline page + * message) from the same classification, via `describeDevFolderError`. */ +export type SetDevFolderResult = + | { kind: 'ok'; actor: ActorRecord } + | { kind: 'invalid-path'; message: string } + | { kind: 'no-successful-build' } + | { kind: 'unreachable' } + | { kind: 'image-missing' } + | { kind: 'not-found' } + | { kind: 'unknown' }; + +/** + * The one validate-and-persist path both the API endpoint and the console form funnel through + * (`design.md`: "Both paths funnel into one service function that validates and persists"). `path` is + * already unwrapped from its transport encoding and trimmed by the caller. + * + * An empty `path` is a first-class "clear" operation - it always succeeds, never runs the shape check, + * the build-first check, or the existence probe (`design.md`: "Clearing... never runs the existence + * check, since there is no path to check"). A non-empty `path` must pass the shape pre-filter, then + * requires the Actor to have at least one successful build (so there is an image to probe against at + * all), then must pass the host-side existence probe - in that order, each one short-circuiting the + * next on failure. A rejected call never touches `updateActor` at all, so a previously-registered value + * survives untouched across a later failed registration attempt (success criterion 8). + */ +export async function setDevFolder(driver: Driver, actor: ActorRecord, path: string): Promise { + if (path === '') { + const updated = await updateActor(actor.id, (current) => ({ ...current, localDevFolder: undefined })); + return { kind: 'ok', actor: updated ?? { ...actor, localDevFolder: undefined } }; + } + + const shapeError = validateDevFolderPathShape(path); + if (shapeError) return { kind: 'invalid-path', message: shapeError }; + + const imageId = await resolveProbeImageId(actor); + if (!imageId) return { kind: 'no-successful-build' }; + + const probe: DevFolderProbeOutcome = driver.probeDevFolder + ? await driver.probeDevFolder(path, imageId) + : { ok: false, reason: 'unreachable' }; + if (!probe.ok) return { kind: probe.reason }; + + const updated = await updateActor(actor.id, (current) => ({ ...current, localDevFolder: path })); + return { kind: 'ok', actor: updated ?? { ...actor, localDevFolder: path } }; +} + +export interface DevFolderErrorInfo { + status: number; + type: string; + message: string; +} + +/** Maps every non-`ok` `SetDevFolderResult` to the status/type/message the API route wraps in an + * `ApiError` and the console form renders inline - one mapping, two presentations (`design.md`'s error + * classification, most specific first: unreachable/image-missing/not-found/unknown). */ +export function describeDevFolderError(result: Exclude): DevFolderErrorInfo { + switch (result.kind) { + case 'invalid-path': + return { status: 400, type: 'invalid-request', message: result.message }; + case 'no-successful-build': + return { + status: 400, + type: 'dev-folder-not-buildable', + message: 'This Actor has no successful build yet - push and build it before registering a dev folder.', + }; + case 'not-found': + return { + status: 400, + type: 'dev-folder-path-not-found', + message: 'The submitted path does not exist on the host.', + }; + case 'unreachable': + return { + status: 503, + type: 'dev-folder-check-unavailable', + message: 'Could not verify the path - Docker is unreachable.', + }; + case 'image-missing': + return { + status: 500, + type: 'internal-error', + message: 'Could not verify the path - internal error (the build image is missing).', + }; + case 'unknown': + return { + status: 400, + type: 'dev-folder-check-failed', + message: 'Could not verify this path.', + }; + } +} + +export interface DevFolderStatus { + localDevFolder: string | null; + imageWorkingDirectory: string | null; + /** Whether `startRun` will actually add the bind mount on this Actor's next run - `true` only when + * both fields are present and non-empty (`design.md`: "No mount is added when either field is + * missing or the folder is empty"). Shown separately from the two raw fields so the console/API + * caller never has to re-derive this condition themselves (success criterion 28). */ + mountWillApply: boolean; +} + +/** The three values both the API's registration response and the console detail page show - one + * derivation, so they can never drift apart (success criterion 27). */ +export function devFolderStatus(actor: ActorRecord): DevFolderStatus { + const localDevFolder = actor.localDevFolder ?? null; + const imageWorkingDirectory = actor.imageWorkingDirectory ?? null; + return { + localDevFolder, + imageWorkingDirectory, + mountWillApply: Boolean(localDevFolder) && Boolean(imageWorkingDirectory), + }; +} diff --git a/src/services/builds.ts b/src/services/builds.ts index cd6db10..cf7e825 100644 --- a/src/services/builds.ts +++ b/src/services/builds.ts @@ -192,9 +192,21 @@ export async function runBuildInBackground( // `apify call`/`POST .../runs` against that tag even though the build record itself correctly // stayed ABORTED. if (succeeded?.status === 'SUCCEEDED') { - await updateActor(actor.id, (current) => - recordTaggedBuild(current, options.tag, record.id, record.buildNumber), - ); + // Folded into the same `updateActor` call that records the tagged build (`design.md`'s + // "Capture of `imageWorkingDirectory`"), so it lands in `__ACTORS__` in one write, not two. Only + // set when this build's inspect actually produced a value: `outcome.imageWorkingDirectory` is + // `undefined` both when the inspect call itself failed and when the image's working directory was + // empty/`/` (`docker-driver.ts`'s `inspectWorkingDirectory`) - either way, that must never fail the + // (otherwise-successful) build, and it must not clobber a previously known-good value with + // `undefined` just because *this* build's inspect happened to come up empty (`design.md`'s "Stale + // working directory" risk already accepts the field reflecting an older build; overwriting a known + // value with "unset" on a transient inspect hiccup would be strictly worse than that, not better). + await updateActor(actor.id, (current) => { + const withTag = recordTaggedBuild(current, options.tag, record.id, record.buildNumber); + return outcome.imageWorkingDirectory !== undefined + ? { ...withTag, imageWorkingDirectory: outcome.imageWorkingDirectory } + : withTag; + }); } } catch (error) { const status: JobStatus = error instanceof DriverTimedOutError ? 'TIMED-OUT' : 'FAILED'; diff --git a/src/services/runs.ts b/src/services/runs.ts index 027c84e..6684702 100644 --- a/src/services/runs.ts +++ b/src/services/runs.ts @@ -207,6 +207,16 @@ export async function runInBackground( const version = findVersion(actor, build.versionNumber); const env = buildEnv(record, actor, version, options); + // Both-or-neither, enforced by `DevFolderMount`'s type (`driver/types.ts`) - a mount is only ever + // added when the Actor actually has a non-empty registered dev folder AND a known, non-empty image + // working directory (`design.md`: "No mount is added when either field is missing or the folder is + // empty"). An Actor that was never registered (or was cleared) gets `devMount: undefined`, which + // `docker-driver.ts`'s `startRun` treats identically to "no `Mounts` key at all" - the regression + // guarantee (success criterion 23). + const devMount = + actor.localDevFolder && actor.imageWorkingDirectory + ? { localDevFolder: actor.localDevFolder, imageWorkingDirectory: actor.imageWorkingDirectory } + : undefined; // Re-check right before creating the container: an abort issued while the registry/version lookups // above were in flight may have already moved the record to ABORTING. Closing this window is the fix @@ -229,6 +239,7 @@ export async function runInBackground( env, memoryMbytes: record.options.memoryMbytes, timeoutSecs: record.options.timeoutSecs, + devMount, }, (chunk) => appendLog(record.id, chunk), ); diff --git a/src/storage/entities.ts b/src/storage/entities.ts index 0f7f9a3..3af507f 100644 --- a/src/storage/entities.ts +++ b/src/storage/entities.ts @@ -60,6 +60,28 @@ export interface ActorRecord { versions: ActorVersionRecord[]; /** tag -> latest successful build for that tag; `apify push` polls this after a build. */ taggedBuilds: Record; + /** + * The host path bind-mounted over the image's working directory at run start, for rapid local dev + * without a rebuild (`actor-driver.md`'s "Bind mount volumes with Actor source code"). Set/cleared + * only through `services/actors.ts: setDevFolder` (the API's `POST /actor-runtime/dev-folder/:actorId` + * and the console's dev-folder form both funnel through it) - never touched by any other Actor write + * (`storage.md`). Absent (never registered) and present-but-empty are not distinguished on this type; + * `setDevFolder` always stores either a non-empty absolute path or clears the key entirely via + * `undefined` (which a JSON round-trip through the KV store drops), so in practice this is only ever + * "absent" or "a real path" - never an empty string at rest. Optional and never exposed on `/v2` + * (`dto/actors.ts: actorDto` is explicit field-by-field). + */ + localDevFolder?: string; + /** + * The Actor's most recently successfully-built image's `Config.WorkingDir`, captured by the driver + * right after that build (`docker-driver.ts`'s `startBuild`) and persisted in the same `updateActor` + * call that records the tagged build (`services/builds.ts`). Optional: unset until at least one build + * has succeeded and its image could be inspected, and left unset (not overwritten) by a build whose + * inspect failed or whose image's working directory was empty/`/` (`design.md`: mounting over `/` + * would destroy the container). Reflects the *most recent* successful build only - see `design.md`'s + * "Stale working directory" risk. Optional and never exposed on `/v2`, same as `localDevFolder`. + */ + imageWorkingDirectory?: string; } export type JobStatus = 'READY' | 'RUNNING' | 'SUCCEEDED' | 'FAILED' | 'ABORTING' | 'ABORTED' | 'TIMED-OUT'; diff --git a/test/e2e/dev-folder-bind-mount.test.ts b/test/e2e/dev-folder-bind-mount.test.ts new file mode 100644 index 0000000..edb089a --- /dev/null +++ b/test/e2e/dev-folder-bind-mount.test.ts @@ -0,0 +1,245 @@ +/** + * E2E case for the local dev-folder bind mount (`design.md`'s Testability section, "Only the real-Docker + * e2e suite can prove a genuine host path passes the probe and the mount itself"): after one real + * push+build, registering the Actor's host source folder via the documented + * `apify api POST ../actor-runtime/dev-folder/` invocation and then recompiling *locally* must + * be picked up by the *next* `apify call`, with no intervening `apify push`/build - and the image's own + * `node_modules` must survive the mount (the anonymous-volume guarantee). Registration is itself an + * `apify` command, so `requirements/test.md`'s CLI-only rule needs no exception here. + * + * Requires a reachable Docker daemon and fails loudly, never skips, mirroring `actor-dev-loop.test.ts`. + */ +import { execFileSync } from 'node:child_process'; +import { readFileSync, writeFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { dirname, join } from 'node:path'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; + +import { + buildRuntimeImage, + isDockerAvailable, + pullBaseImages, + startRuntimeContainer, + stopRuntimeContainer, + waitForHttpOk, +} from './helpers/docker.js'; +import { + apify, + apifyAllOutput, + apifyEnv, + createIsolatedApifyHome, + loginApifyCli, + removeIsolatedApifyHome, + type CallResult, + type PushResult, +} from './helpers/apify-cli.js'; + +const __dirname = dirname(fileURLToPath(import.meta.url)); +const REPO_ROOT = join(__dirname, '..', '..'); +const ACTOR_DIR = join(REPO_ROOT, 'sample_actor_ts'); +const MAIN_TS = join(ACTOR_DIR, 'src', 'main.ts'); +const CONTAINER_NAME = 'actor-runtime-e2e-devfolder'; +const IMAGE_TAG = 'actor-runtime:e2e-devfolder'; +// `sample_actor_ts/Dockerfile` sets no `WORKDIR` of its own, so it inherits the base image's +// (`apify/actor-node`'s own Dockerfile - confirmed live against its current default branch in +// `.shepherd/_request_fact_check.md`'s round-1 claim 7). Asserted independently below via the +// registration response's own `imageWorkingDirectory`, not only assumed here - if the base image ever +// moves its `WORKDIR`, that assertion (not the mount itself) is what will fail first and explain why. +const EXPECTED_IMAGE_WORKING_DIR = '/usr/src/app'; +const ORIGINAL_MARKER = 'Crawl finished.'; +const EDITED_MARKER = 'Crawl finished (dev-folder-edit-marker).'; + +/** `apify api POST ...`'s printed `{ data: ... }` envelope for this endpoint's response shape + * (`services/actors.ts: devFolderStatus`). */ +interface DevFolderApiResult { + data: { localDevFolder: string | null; imageWorkingDirectory: string | null; mountWillApply: boolean }; +} + +function registerDevFolder(actorId: string, path: string, env: NodeJS.ProcessEnv): DevFolderApiResult { + // The exact CLI invocation the product description promises, escaping the CLI's own `/v2` base + // (`design.md`'s Decision #5) - `cwd: REPO_ROOT` matters here since `../actor-runtime/...` resolves + // against the CLI's configured base URL, not the filesystem; it is unrelated to `path` itself, which + // is always this test's own absolute `ACTOR_DIR`. + const output = apify(['api', 'POST', `../actor-runtime/dev-folder/${actorId}`, '--body', JSON.stringify(path)], { + cwd: REPO_ROOT, + env, + }); + return JSON.parse(output) as DevFolderApiResult; +} + +describe('local dev-folder bind mount: edit-compile-call loop with no rebuild (requires Docker)', () => { + let isolatedApifyHome: string; + let originalMainTs: string | undefined; + + beforeAll( + async () => { + if (!isDockerAvailable()) { + throw new Error( + 'Docker daemon is not reachable - this e2e case requires one (see requirements/test.md)', + ); + } + + pullBaseImages(); + buildRuntimeImage(REPO_ROOT, IMAGE_TAG); + startRuntimeContainer(IMAGE_TAG, CONTAINER_NAME); + await waitForHttpOk('http://localhost:3333/v2/users/me?token=x'); + + isolatedApifyHome = createIsolatedApifyHome(); + loginApifyCli(REPO_ROOT, isolatedApifyHome); + + originalMainTs = readFileSync(MAIN_TS, 'utf8'); + // A genuine local compile needs the sample Actor's own devDependencies (`typescript`) present on + // the *host* - distinct from what `apify push` sends the runtime (source files only; the + // runtime's own Docker build installs and compiles them again, inside the image). + execFileSync('npm', ['install'], { cwd: ACTOR_DIR, stdio: 'inherit' }); + }, + 10 * 60 * 1000, + ); + + afterAll(() => { + // Best-effort: restore src/ and dist/ to what they were before this suite touched them, so a + // later local run of `actor-dev-loop.test.ts` (or a human) doesn't inherit the edited marker. + // Guarded the same way `isolatedApifyHome` below is - `beforeAll` can throw before either is ever + // assigned (e.g. the Docker-unreachable check at its top). + if (originalMainTs !== undefined) { + writeFileSync(MAIN_TS, originalMainTs); + try { + execFileSync('npm', ['run', 'build'], { cwd: ACTOR_DIR, stdio: 'ignore' }); + } catch { + // best-effort only + } + } + stopRuntimeContainer(CONTAINER_NAME); + if (isolatedApifyHome) removeIsolatedApifyHome(isolatedApifyHome); + }); + + it( + 'registers the host folder, then a local recompile (no push/build) is what the next run sees, with node_modules preserved', + async () => { + const env = apifyEnv(isolatedApifyHome); + + // One real push + build, as the product description requires ("push and build once, then + // register") - the build-first precondition (`requirements/api.md`). + const pushOutput = apify(['push', '--json'], { cwd: ACTOR_DIR, env }); + const push = JSON.parse(pushOutput) as PushResult; + expect(push.build.status).toBe('SUCCEEDED'); + const actorId = push.actor.id; + + // Local build, so the host folder already looks like the image's working directory before it + // is ever bind-mounted over it (`design.md`'s "Layout mismatch" risk) - `dist/main.js` for the + // container's own `CMD` to run. The host folder's own `node_modules` (just installed above) is + // deliberately NOT what the container is meant to rely on - the assertion below proves the + // anonymous volume, not this directory's own `node_modules`, is what the container actually used. + execFileSync('npm', ['run', 'build'], { cwd: ACTOR_DIR, stdio: 'inherit' }); + + const registered = registerDevFolder(actorId, ACTOR_DIR, env); + expect(registered.data.localDevFolder).toBe(ACTOR_DIR); + expect(registered.data.imageWorkingDirectory).toBe(EXPECTED_IMAGE_WORKING_DIR); + expect(registered.data.mountWillApply).toBe(true); + + // Edit the source, recompile locally - deliberately no `apify push`/`apify build` between here + // and the `apify call` below, which is the entire point of the feature. + writeFileSync(MAIN_TS, originalMainTs!.replace(ORIGINAL_MARKER, EDITED_MARKER)); + execFileSync('npm', ['run', 'build'], { cwd: ACTOR_DIR, stdio: 'inherit' }); + + const callOutput = apify(['call', '--input', JSON.stringify({ maxPages: 1 }), '--json'], { + cwd: ACTOR_DIR, + env, + }); + const call = JSON.parse(callOutput) as CallResult; + // The dependencies (`apify`, `@crawlee/cheerio`) still resolved and the crawl actually ran - + // proving the anonymous `node_modules` volume preserved the image's own installed packages, + // even though the bind mount just replaced the whole working directory with the host folder. + expect(call.run.status).toBe('SUCCEEDED'); + + const log = apifyAllOutput(['runs', 'log', call.run.id], { cwd: REPO_ROOT, env }); + // The recompiled marker line, not the original - proves the container's working directory came + // from the freshly-recompiled host folder, not the image's originally-baked-in `dist/`. + expect(log).toContain(EDITED_MARKER); + expect(log).not.toContain(`${ORIGINAL_MARKER}\n`); + + // An explicit mount line at the top of the run's log (`design.md`'s "Observability of the + // mount"), naming both the host path and the container path being mounted. + expect(log).toContain(ACTOR_DIR); + expect(log).toContain(EXPECTED_IMAGE_WORKING_DIR); + }, + 5 * 60 * 1000, + ); + + it( + 'clearing the registration (empty JSON string) makes the next run see the image again, not the host folder', + async () => { + const env = apifyEnv(isolatedApifyHome); + + const pushOutput = apify(['push', '--json'], { cwd: ACTOR_DIR, env }); + const push = JSON.parse(pushOutput) as PushResult; + const actorId = push.actor.id; + + registerDevFolder(actorId, ACTOR_DIR, env); + + const clearOutput = apify(['api', 'POST', `../actor-runtime/dev-folder/${actorId}`, '--body', '""'], { + cwd: REPO_ROOT, + env, + }); + const cleared = JSON.parse(clearOutput) as DevFolderApiResult; + expect(cleared.data.localDevFolder).toBeNull(); + expect(cleared.data.mountWillApply).toBe(false); + + // The edited marker from a previous test in this file (if it ran first) or the host `dist/` + // otherwise must NOT be what this run sees - restore the original source/build first so this + // case is self-contained regardless of test order. + writeFileSync(MAIN_TS, originalMainTs!); + execFileSync('npm', ['run', 'build'], { cwd: ACTOR_DIR, stdio: 'inherit' }); + + const callOutput = apify(['call', '--input', JSON.stringify({ maxPages: 1 }), '--json'], { + cwd: ACTOR_DIR, + env, + }); + const call = JSON.parse(callOutput) as CallResult; + expect(call.run.status).toBe('SUCCEEDED'); + + const log = apifyAllOutput(['runs', 'log', call.run.id], { cwd: REPO_ROOT, env }); + // No mount line at all - the observability line only appears for a run that actually has one. + expect(log).not.toContain('Mounting local dev folder'); + }, + 5 * 60 * 1000, + ); + + it( + 'anonymous node_modules volumes do not accumulate across runs ({ v: true } cleanup)', + async () => { + const env = apifyEnv(isolatedApifyHome); + + const pushOutput = apify(['push', '--json'], { cwd: ACTOR_DIR, env }); + const push = JSON.parse(pushOutput) as PushResult; + const actorId = push.actor.id; + + writeFileSync(MAIN_TS, originalMainTs!); + execFileSync('npm', ['run', 'build'], { cwd: ACTOR_DIR, stdio: 'inherit' }); + registerDevFolder(actorId, ACTOR_DIR, env); + + // Dangling (unattached) volumes on the whole daemon - a coarse but simple proxy: this suite is + // the only thing exercising anonymous volumes against this daemon in a CI run, so a stable count + // across repeated runs is sufficient evidence the driver's `{ v: true }` cleanup (not some + // unrelated daemon-wide accumulation) is what's being measured. + const countDanglingVolumes = (): number => + execFileSync('docker', ['volume', 'ls', '-q', '-f', 'dangling=true'], { encoding: 'utf8' }) + .split('\n') + .filter((line) => line.trim().length > 0).length; + + const before = countDanglingVolumes(); + for (let i = 0; i < 3; i++) { + const callOutput = apify(['call', '--input', JSON.stringify({ maxPages: 1 }), '--json'], { + cwd: ACTOR_DIR, + env, + }); + const call = JSON.parse(callOutput) as CallResult; + expect(call.run.status).toBe('SUCCEEDED'); + } + const after = countDanglingVolumes(); + + expect(after).toBe(before); + }, + 5 * 60 * 1000, + ); +}); diff --git a/test/integration/console.test.ts b/test/integration/console.test.ts index 862bb53..f89debf 100644 --- a/test/integration/console.test.ts +++ b/test/integration/console.test.ts @@ -20,7 +20,7 @@ describe('console pages (HTTP fetch)', () => { beforeEach(async () => { server = await startTestServer(); - const app = createConsoleServer(); + const app = createConsoleServer({ driver: server.driver }); consoleServer = await new Promise((resolve) => { const s = app.listen(0, () => resolve(s)); }); diff --git a/test/integration/dev-folder.test.ts b/test/integration/dev-folder.test.ts new file mode 100644 index 0000000..d0ef47a --- /dev/null +++ b/test/integration/dev-folder.test.ts @@ -0,0 +1,497 @@ +/** + * Integration coverage for the local dev-folder bind-mount feature's non-Docker-dependent surface + * (`design.md`): the API endpoint's auth/ownership/shape/build-first/probe-classification contract, + * that the registered value never leaks into any `/v2` Actor response, and the console's single-field + * form (render, submit, clear, redirect, inline error). Every probe outcome is stubbed + * (`devFolderDriver` below) - there is no Docker daemon in this sandbox (`docker-driver.ts`'s class doc + * comment); the real-probe accept/reject path and the mount itself are only exercised end-to-end in + * `test/e2e/dev-folder-bind-mount.test.ts`. + */ +import type { AddressInfo } from 'node:net'; +import type { Server } from 'node:http'; +import { afterEach, describe, expect, it } from 'vitest'; +import axios from 'axios'; + +import { startTestServer, type TestServerHandle } from './helpers/test-server.js'; +import { createConsoleServer } from '../../src/console/server.js'; +import { getRegistries } from '../../src/storage/registries.js'; +import { generateId } from '../../src/storage/ids.js'; +import { recordTaggedBuild, updateActor } from '../../src/services/actors.js'; +import type { ActorRecord, BuildRecord } from '../../src/storage/entities.js'; +import type { Driver, DevFolderProbeOutcome } from '../../src/driver/types.js'; + +/** + * A `Driver` whose only interesting behaviour is `probeDevFolder`, returning a caller-controlled + * outcome that can be changed mid-test via `setOutcome` (for the "a prior registration survives a later + * failed attempt" contract, which needs the same driver to succeed once and then fail). `probeDevFolderCalls` + * records every `(candidatePath, imageId)` pair it was called with, so a test can assert the probe was - + * or, for the "clearing never checks" contract, was NOT - invoked at all. + */ +function devFolderDriver( + initialOutcome: DevFolderProbeOutcome, + available = true, +): Driver & { probeDevFolderCalls: Array<[string, string]>; setOutcome(next: DevFolderProbeOutcome): void } { + let outcome = initialOutcome; + const probeDevFolderCalls: Array<[string, string]> = []; + return { + available, + probeDevFolderCalls, + setOutcome(next: DevFolderProbeOutcome) { + outcome = next; + }, + async init() {}, + async startBuild() { + throw new Error('not used by this stub'); + }, + async abortBuild() {}, + async startRun() { + throw new Error('not used by this stub'); + }, + async abortRun() {}, + async reconcileOrphans() {}, + async probeDevFolder(candidatePath: string, imageId: string) { + probeDevFolderCalls.push([candidatePath, imageId]); + return outcome; + }, + }; +} + +/** A SUCCEEDED build with a fake image, tagged against the actor - the build-first precondition's + * "happy path" state (mirrors `job-lifecycle.test.ts`'s identical helper). */ +async function seedSucceededBuild(actor: ActorRecord, tag = 'latest'): Promise { + const build: BuildRecord = { + id: generateId(), + userId: actor.userId, + actorId: actor.id, + versionNumber: '0.0', + buildNumber: '0.0.1', + tag, + status: 'SUCCEEDED', + startedAt: new Date().toISOString(), + finishedAt: new Date().toISOString(), + imageId: 'fake-image:latest', + }; + await getRegistries().builds.set(build.id, build); + await updateActor(actor.id, (current) => recordTaggedBuild(current, tag, build.id, build.buildNumber)); + return build; +} + +function post(baseUrl: string, actorId: string, body: string, token?: string) { + return axios.post(`${baseUrl}/actor-runtime/dev-folder/${actorId}`, body, { + headers: token ? { Authorization: `Bearer ${token}` } : {}, + validateStatus: () => true, + }); +} + +describe('POST /actor-runtime/dev-folder/:actorId', () => { + let server: TestServerHandle; + + afterEach(async () => { + await server.close(); + }); + + it('401s with no auth token', async () => { + server = await startTestServer(devFolderDriver({ ok: true })); + const res = await post(server.baseUrl, 'whatever-id', JSON.stringify('/abs/path')); + expect(res.status).toBe(401); + }); + + it("404s for an actor id that doesn't exist", async () => { + server = await startTestServer(devFolderDriver({ ok: true })); + const res = await post(server.baseUrl, 'totally-made-up-id', JSON.stringify('/abs/path'), server.token); + expect(res.status).toBe(404); + }); + + it("404s for another user's actor (ownership-scoped, like every other Actor write on this port)", async () => { + server = await startTestServer(devFolderDriver({ ok: true })); + const actor = await server.client.actors().create({ name: 'other-users-actor' }); + + const res = await post(server.baseUrl, actor.id, JSON.stringify('/abs/path'), 'a-completely-different-token'); + expect(res.status).toBe(404); + }); + + it('resolves the actor by plain name and by username~name, not only by id', async () => { + server = await startTestServer(devFolderDriver({ ok: true })); + const actor = await server.client.actors().create({ name: 'name-resolution-actor' }); + await seedSucceededBuild((await getRegistries().actors.get(actor.id))!); + const me = await server.client.user('me').get(); + + const byName = await post(server.baseUrl, actor.name, JSON.stringify('/abs/path-a'), server.token); + expect(byName.status).toBe(200); + + const byUsernameTilde = await post( + server.baseUrl, + `${me.username}~${actor.name}`, + JSON.stringify('/abs/path-b'), + server.token, + ); + expect(byUsernameTilde.status).toBe(200); + expect(byUsernameTilde.data.data.localDevFolder).toBe('/abs/path-b'); + }); + + it('400s for a body that is not valid JSON at all', async () => { + server = await startTestServer(devFolderDriver({ ok: true })); + const actor = await server.client.actors().create({ name: 'malformed-body-actor' }); + const res = await post(server.baseUrl, actor.id, 'not-json-at-all', server.token); + expect(res.status).toBe(400); + }); + + it('400s for a body that is valid JSON but not a string (e.g. a bare number)', async () => { + server = await startTestServer(devFolderDriver({ ok: true })); + const actor = await server.client.actors().create({ name: 'non-string-body-actor' }); + const res = await post(server.baseUrl, actor.id, JSON.stringify(42), server.token); + expect(res.status).toBe(400); + }); + + it('400s for a relative path, even for an actor with a successful build, and stores nothing', async () => { + server = await startTestServer(devFolderDriver({ ok: true })); + const actor = await server.client.actors().create({ name: 'relative-path-actor' }); + await seedSucceededBuild((await getRegistries().actors.get(actor.id))!); + + const res = await post(server.baseUrl, actor.id, JSON.stringify('relative/path'), server.token); + expect(res.status).toBe(400); + const stored = await getRegistries().actors.get(actor.id); + expect(stored?.localDevFolder).toBeUndefined(); + }); + + it('400s for a non-empty path when the actor has never had a successful build, and never even calls the probe', async () => { + const driver = devFolderDriver({ ok: true }); + server = await startTestServer(driver); + const actor = await server.client.actors().create({ name: 'never-built-actor' }); + + const res = await post(server.baseUrl, actor.id, JSON.stringify('/abs/path'), server.token); + expect(res.status).toBe(400); + expect(driver.probeDevFolderCalls).toEqual([]); + + const stored = await getRegistries().actors.get(actor.id); + expect(stored?.localDevFolder).toBeUndefined(); + }); + + it('rejects the same way for an actor whose only build attempt failed (no successful build ever)', async () => { + server = await startTestServer(devFolderDriver({ ok: true })); + const actor = await server.client.actors().create({ name: 'failed-build-only-actor' }); + const actorRecord = (await getRegistries().actors.get(actor.id))!; + await getRegistries().builds.set(generateId(), { + id: generateId(), + userId: actorRecord.userId, + actorId: actor.id, + versionNumber: '0.0', + buildNumber: '0.0.1', + tag: 'latest', + status: 'FAILED', + startedAt: new Date().toISOString(), + finishedAt: new Date().toISOString(), + }); + + const res = await post(server.baseUrl, actor.id, JSON.stringify('/abs/path'), server.token); + expect(res.status).toBe(400); + }); + + it('200s and stores the path when the probe reports ok, for an actor with a successful build', async () => { + const driver = devFolderDriver({ ok: true }); + server = await startTestServer(driver); + const actor = await server.client.actors().create({ name: 'happy-path-actor' }); + const build = await seedSucceededBuild((await getRegistries().actors.get(actor.id))!); + + const res = await post(server.baseUrl, actor.id, JSON.stringify('/abs/path/to/src'), server.token); + expect(res.status).toBe(200); + expect(res.data.data.localDevFolder).toBe('/abs/path/to/src'); + expect(driver.probeDevFolderCalls).toEqual([['/abs/path/to/src', build.imageId]]); + + const stored = await getRegistries().actors.get(actor.id); + expect(stored?.localDevFolder).toBe('/abs/path/to/src'); + }); + + it('the response reports the detected imageWorkingDirectory and whether a mount will apply', async () => { + server = await startTestServer(devFolderDriver({ ok: true })); + const actor = await server.client.actors().create({ name: 'status-fields-actor' }); + await seedSucceededBuild((await getRegistries().actors.get(actor.id))!); + await updateActor(actor.id, (current) => ({ ...current, imageWorkingDirectory: '/usr/src/app' })); + + const res = await post(server.baseUrl, actor.id, JSON.stringify('/abs/path'), server.token); + expect(res.data.data).toEqual({ + localDevFolder: '/abs/path', + imageWorkingDirectory: '/usr/src/app', + mountWillApply: true, + }); + }); + + it('a second registration replaces the first outright, not merges', async () => { + server = await startTestServer(devFolderDriver({ ok: true })); + const actor = await server.client.actors().create({ name: 'replace-actor' }); + await seedSucceededBuild((await getRegistries().actors.get(actor.id))!); + + await post(server.baseUrl, actor.id, JSON.stringify('/abs/first'), server.token); + const second = await post(server.baseUrl, actor.id, JSON.stringify('/abs/second'), server.token); + + expect(second.data.data.localDevFolder).toBe('/abs/second'); + const stored = await getRegistries().actors.get(actor.id); + expect(stored?.localDevFolder).toBe('/abs/second'); + }); + + it('registering for Actor A never appears on Actor B', async () => { + server = await startTestServer(devFolderDriver({ ok: true })); + const actorA = await server.client.actors().create({ name: 'cross-actor-a' }); + const actorB = await server.client.actors().create({ name: 'cross-actor-b' }); + await seedSucceededBuild((await getRegistries().actors.get(actorA.id))!); + + await post(server.baseUrl, actorA.id, JSON.stringify('/abs/only-a'), server.token); + + const storedB = await getRegistries().actors.get(actorB.id); + expect(storedB?.localDevFolder).toBeUndefined(); + }); + + it('unrelated Actor writes (e.g. PUT name/title) never touch a previously-registered dev folder', async () => { + server = await startTestServer(devFolderDriver({ ok: true })); + const actor = await server.client.actors().create({ name: 'unrelated-write-actor' }); + await seedSucceededBuild((await getRegistries().actors.get(actor.id))!); + await post(server.baseUrl, actor.id, JSON.stringify('/abs/untouched'), server.token); + + await server.client.actor(actor.id).update({ title: 'a new title' }); + + const stored = await getRegistries().actors.get(actor.id); + expect(stored?.localDevFolder).toBe('/abs/untouched'); + }); + + it('200s and clears with the empty JSON string, without the clear itself ever calling the probe', async () => { + const driver = devFolderDriver({ ok: true }); + server = await startTestServer(driver); + const actor = await server.client.actors().create({ name: 'clear-actor' }); + await seedSucceededBuild((await getRegistries().actors.get(actor.id))!); + await post(server.baseUrl, actor.id, JSON.stringify('/abs/path'), server.token); + expect(driver.probeDevFolderCalls.length).toBe(1); + + const res = await post(server.baseUrl, actor.id, JSON.stringify(''), server.token); + expect(res.status).toBe(200); + expect(res.data.data.localDevFolder).toBeNull(); + expect(driver.probeDevFolderCalls.length).toBe(1); // unchanged - clearing never probes + + const stored = await getRegistries().actors.get(actor.id); + expect(stored?.localDevFolder).toBeUndefined(); + }); + + it('clearing succeeds even when Docker is unreachable (unavailable driver) - no existence check needed', async () => { + server = await startTestServer(devFolderDriver({ ok: false, reason: 'unreachable' }, false)); + const actor = await server.client.actors().create({ name: 'clear-unreachable-actor' }); + + const res = await post(server.baseUrl, actor.id, JSON.stringify(''), server.token); + expect(res.status).toBe(200); + expect(res.data.data.localDevFolder).toBeNull(); + }); + + it('a prior valid registration survives untouched across a later failed registration attempt', async () => { + const driver = devFolderDriver({ ok: true }); + server = await startTestServer(driver); + const actor = await server.client.actors().create({ name: 'survive-actor' }); + await seedSucceededBuild((await getRegistries().actors.get(actor.id))!); + + const first = await post(server.baseUrl, actor.id, JSON.stringify('/abs/good-path'), server.token); + expect(first.status).toBe(200); + + driver.setOutcome({ ok: false, reason: 'not-found' }); + const second = await post(server.baseUrl, actor.id, JSON.stringify('/abs/bad-path'), server.token); + expect(second.status).toBe(400); + + const stored = await getRegistries().actors.get(actor.id); + expect(stored?.localDevFolder).toBe('/abs/good-path'); + }); + + it('classifies a not-found probe outcome as 400 "does not exist", and stores nothing', async () => { + server = await startTestServer(devFolderDriver({ ok: false, reason: 'not-found' })); + const actor = await server.client.actors().create({ name: 'not-found-actor' }); + await seedSucceededBuild((await getRegistries().actors.get(actor.id))!); + + const res = await post(server.baseUrl, actor.id, JSON.stringify('/abs/missing'), server.token); + expect(res.status).toBe(400); + expect(res.data.error.message.toLowerCase()).toContain('does not exist'); + + const stored = await getRegistries().actors.get(actor.id); + expect(stored?.localDevFolder).toBeUndefined(); + }); + + it('classifies an unreachable probe outcome as 503, distinguishable from "does not exist"', async () => { + server = await startTestServer(devFolderDriver({ ok: false, reason: 'unreachable' })); + const actor = await server.client.actors().create({ name: 'unreachable-actor' }); + await seedSucceededBuild((await getRegistries().actors.get(actor.id))!); + + const res = await post(server.baseUrl, actor.id, JSON.stringify('/abs/path'), server.token); + expect(res.status).toBe(503); + expect(res.data.error.message.toLowerCase()).not.toContain('does not exist'); + }); + + it('classifies an image-missing probe outcome as 500 internal error, distinguishable from "does not exist"', async () => { + server = await startTestServer(devFolderDriver({ ok: false, reason: 'image-missing' })); + const actor = await server.client.actors().create({ name: 'image-missing-actor' }); + await seedSucceededBuild((await getRegistries().actors.get(actor.id))!); + + const res = await post(server.baseUrl, actor.id, JSON.stringify('/abs/path'), server.token); + expect(res.status).toBe(500); + expect(res.data.error.message.toLowerCase()).not.toContain('does not exist'); + }); + + it('classifies an unknown mount-shaped rejection as 400 "could not verify", never "does not exist"', async () => { + server = await startTestServer(devFolderDriver({ ok: false, reason: 'unknown' })); + const actor = await server.client.actors().create({ name: 'unknown-reason-actor' }); + await seedSucceededBuild((await getRegistries().actors.get(actor.id))!); + + const res = await post(server.baseUrl, actor.id, JSON.stringify('/abs/path'), server.token); + expect(res.status).toBe(400); + expect(res.data.error.message.toLowerCase()).not.toContain('does not exist'); + }); + + it('the registered value never appears in any /v2 Actor response (list or get)', async () => { + server = await startTestServer(devFolderDriver({ ok: true })); + const actor = await server.client.actors().create({ name: 'no-leak-actor' }); + await seedSucceededBuild((await getRegistries().actors.get(actor.id))!); + await post(server.baseUrl, actor.id, JSON.stringify('/abs/should-not-leak'), server.token); + + const fetched = await server.client.actor(actor.id).get(); + expect(JSON.stringify(fetched)).not.toContain('/abs/should-not-leak'); + expect(fetched).not.toHaveProperty('localDevFolder'); + expect(fetched).not.toHaveProperty('imageWorkingDirectory'); + + const listed = await server.client.actors().list(); + expect(JSON.stringify(listed)).not.toContain('/abs/should-not-leak'); + }); +}); + +describe('console: dev-folder registration form on the Actor detail view', () => { + let server: TestServerHandle; + let consoleServer: Server; + let consoleBaseUrl: string; + + async function setUpConsole(driver: Driver): Promise { + // The API server's own driver is irrelevant to these tests (only the console's dev-folder route + // is exercised here), but `startTestServer` needs one to create Actors through `server.client` - + // reusing the same instance keeps this simple and means `probeDevFolderCalls`/`setOutcome` (if the + // caller passed a `devFolderDriver`) are also visible through `server.driver`. + server = await startTestServer(driver); + const app = createConsoleServer({ driver }); + consoleServer = await new Promise((resolve) => { + const s = app.listen(0, () => resolve(s)); + }); + consoleBaseUrl = `http://127.0.0.1:${(consoleServer.address() as AddressInfo).port}`; + } + + afterEach(async () => { + await new Promise((resolve) => consoleServer.close(() => resolve())); + await server.close(); + }); + + it('renders the three status rows and the form for an Actor with no registration yet', async () => { + await setUpConsole(devFolderDriver({ ok: true })); + const actor = await server.client.actors().create({ name: 'devfolder-render-actor' }); + + const detail = await axios.get(`${consoleBaseUrl}/actors/${actor.id}`); + expect(detail.status).toBe(200); + expect(detail.data).toContain('(none registered)'); + expect(detail.data).toContain('not yet detected'); + expect(detail.data).toContain('mount will apply on the next run'); + expect(detail.data).toContain(`
`); + }); + + it('submitting the form registers the path and redirects back to the detail page, which then shows it', async () => { + const consoleDriver = devFolderDriver({ ok: true }); + await setUpConsole(consoleDriver); + const actor = await server.client.actors().create({ name: 'devfolder-submit-actor' }); + await seedSucceededBuild((await getRegistries().actors.get(actor.id))!); + + const submit = await axios.post( + `${consoleBaseUrl}/actors/${actor.id}/dev-folder`, + 'localDevFolder=%2Fabs%2Fpath', + { + headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, + maxRedirects: 0, + validateStatus: () => true, + }, + ); + expect(submit.status).toBe(302); + expect(submit.headers.location).toBe(`/actors/${actor.id}`); + + const detail = await axios.get(`${consoleBaseUrl}/actors/${actor.id}`); + expect(detail.data).toContain('/abs/path'); + + const stored = await getRegistries().actors.get(actor.id); + expect(stored?.localDevFolder).toBe('/abs/path'); + }); + + it('submitting an empty value clears a previously-registered path', async () => { + await setUpConsole(devFolderDriver({ ok: true })); + const actor = await server.client.actors().create({ name: 'devfolder-clear-actor' }); + await seedSucceededBuild((await getRegistries().actors.get(actor.id))!); + await updateActor(actor.id, (current) => ({ ...current, localDevFolder: '/abs/old-path' })); + + const submit = await axios.post(`${consoleBaseUrl}/actors/${actor.id}/dev-folder`, 'localDevFolder=', { + headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, + maxRedirects: 0, + validateStatus: () => true, + }); + expect(submit.status).toBe(302); + expect(submit.headers.location).toBe(`/actors/${actor.id}`); + + const detail = await axios.get(`${consoleBaseUrl}/actors/${actor.id}`); + expect(detail.data).toContain('(none registered)'); + expect(detail.data).not.toContain('/abs/old-path'); + + const stored = await getRegistries().actors.get(actor.id); + expect(stored?.localDevFolder).toBeUndefined(); + }); + + it('a rejected submission (no successful build) redirects with the classified error surfaced inline, not swallowed', async () => { + await setUpConsole(devFolderDriver({ ok: true })); + const actor = await server.client.actors().create({ name: 'devfolder-error-actor' }); + + const submit = await axios.post( + `${consoleBaseUrl}/actors/${actor.id}/dev-folder`, + 'localDevFolder=%2Fabs%2Fpath', + { + headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, + maxRedirects: 0, + validateStatus: () => true, + }, + ); + expect(submit.status).toBe(302); + expect(submit.headers.location).toContain('devFolderError='); + + const detail = await axios.get(`${consoleBaseUrl}${submit.headers.location}`); + expect(detail.status).toBe(200); + expect(detail.data).toContain('Error'); + expect(detail.data.toLowerCase()).toContain('successful build'); + + // The rejected submission stored nothing. + const stored = await getRegistries().actors.get(actor.id); + expect(stored?.localDevFolder).toBeUndefined(); + }); + + it('the does-not-exist vs. could-not-verify distinction is surfaced on the console too, not collapsed', async () => { + const consoleDriver = devFolderDriver({ ok: false, reason: 'not-found' }); + await setUpConsole(consoleDriver); + const actor = await server.client.actors().create({ name: 'devfolder-not-found-console-actor' }); + await seedSucceededBuild((await getRegistries().actors.get(actor.id))!); + + const submit = await axios.post( + `${consoleBaseUrl}/actors/${actor.id}/dev-folder`, + 'localDevFolder=%2Fabs%2Fmissing', + { + headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, + maxRedirects: 0, + validateStatus: () => true, + }, + ); + const detail = await axios.get(`${consoleBaseUrl}${submit.headers.location}`); + expect(detail.data.toLowerCase()).toContain('does not exist'); + }); + + it('a 404 for a nonexistent Actor id renders Not found, not a 500 or a silent pass', async () => { + await setUpConsole(devFolderDriver({ ok: true })); + const res = await axios.post( + `${consoleBaseUrl}/actors/totally-made-up-id/dev-folder`, + 'localDevFolder=%2Fabs%2Fpath', + { + headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, + validateStatus: () => true, + }, + ); + expect(res.status).toBe(404); + }); +}); diff --git a/test/integration/shutdown.test.ts b/test/integration/shutdown.test.ts index bf77448..75665d5 100644 --- a/test/integration/shutdown.test.ts +++ b/test/integration/shutdown.test.ts @@ -43,7 +43,7 @@ describe('gracefulShutdown', () => { const user = await getOrCreateUserForToken('shutdown-test-token'); const apiApp = createApiServer({ driver: unavailableDriver() }); - const consoleApp = createConsoleServer(); + const consoleApp = createConsoleServer({ driver: unavailableDriver() }); apiServer = await new Promise((resolve) => { const s = apiApp.listen(0, () => resolve(s)); }); diff --git a/test/unit/dev-folder-validation.test.ts b/test/unit/dev-folder-validation.test.ts new file mode 100644 index 0000000..9bfe0cd --- /dev/null +++ b/test/unit/dev-folder-validation.test.ts @@ -0,0 +1,50 @@ +/** + * Pure-function coverage for `validateDevFolderPathShape` (`design.md`: "A cheap shape pre-filter still + * runs first" - absolute POSIX path, no newline/NUL, length cap, `~` never expanded). This is the only + * layer of validation exercisable with no registries/driver at all; `setDevFolder`'s build-first and + * host-side-probe layers are covered by `test/integration/dev-folder.test.ts`. + */ +import { describe, expect, it } from 'vitest'; + +import { validateDevFolderPathShape } from '../../src/services/actors.js'; + +describe('validateDevFolderPathShape', () => { + it('accepts a plain absolute POSIX path', () => { + expect(validateDevFolderPathShape('/home/dev/my-actor')).toBeNull(); + }); + + it('accepts a root-level absolute path', () => { + expect(validateDevFolderPathShape('/src')).toBeNull(); + }); + + it('rejects a relative path', () => { + expect(validateDevFolderPathShape('relative/path')).toMatch(/absolute/i); + }); + + it('does not expand a leading "~" - rejected as non-absolute, not resolved to some assumed home directory', () => { + expect(validateDevFolderPathShape('~/my-actor')).toMatch(/absolute/i); + }); + + it('rejects a path containing a newline', () => { + expect(validateDevFolderPathShape('/home/dev/my\nactor')).toMatch(/newline|NUL/i); + }); + + it('rejects a path containing a carriage return', () => { + expect(validateDevFolderPathShape('/home/dev/my\ractor')).toMatch(/newline|NUL/i); + }); + + it('rejects a path containing a NUL byte', () => { + expect(validateDevFolderPathShape('/home/dev/my\0actor')).toMatch(/newline|NUL/i); + }); + + it('rejects an unreasonably long path', () => { + const long = '/' + 'a'.repeat(5000); + expect(validateDevFolderPathShape(long)).toMatch(/too long/i); + }); + + it('accepts a path right at the length cap boundary', () => { + const atCap = '/' + 'a'.repeat(4095); + expect(atCap.length).toBe(4096); + expect(validateDevFolderPathShape(atCap)).toBeNull(); + }); +}); diff --git a/test/unit/docker-driver.test.ts b/test/unit/docker-driver.test.ts index e669e01..93aeb0d 100644 --- a/test/unit/docker-driver.test.ts +++ b/test/unit/docker-driver.test.ts @@ -12,10 +12,12 @@ import { DockerDriver } from '../../src/driver/docker-driver.js'; */ function stubDocker(containers: Array<{ Id: string; Labels: Record }>) { const removed: string[] = []; + const removeCallOptions: Array | undefined> = []; const listContainers = vi.fn().mockResolvedValue(containers); const getContainer = vi.fn((id: string) => ({ - remove: vi.fn(async () => { + remove: vi.fn(async (options?: Record) => { removed.push(id); + removeCallOptions.push(options); }), })); return { @@ -23,6 +25,7 @@ function stubDocker(containers: Array<{ Id: string; Labels: Record { expect(listContainers).not.toHaveBeenCalled(); }); + + it('removes each matched container with { force: true, v: true } (design.md: Volume cleanup - an orphaned devMount run must not leak its anonymous node_modules volume past a restart)', async () => { + const { docker, removeCallOptions } = stubDocker([ + { Id: 'container-a', Labels: { 'actor-runtime.runId': 'run-a' } }, + ]); + const driver = new DockerDriver(docker); + driver.available = true; + + await driver.reconcileOrphans(['run-a']); + + expect(removeCallOptions).toEqual([{ force: true, v: true }]); + }); }); /** @@ -115,7 +130,7 @@ function stubDockerForRun() { start: vi.fn(async () => undefined), logs: vi.fn(async () => rawLogStream), wait: vi.fn(async () => waitPromise), - remove: vi.fn(async () => undefined), + remove: vi.fn(async (_options?: Record) => undefined), stop: vi.fn(async () => undefined), }; @@ -131,14 +146,16 @@ function stubDockerForRun() { stream.on('data', (chunk: Buffer) => stdout.write(chunk)); }); + const createContainer = vi.fn(async () => container); const docker = { - createContainer: vi.fn(async () => container), + createContainer, modem: { demuxStream }, } as unknown as Docker; return { docker, container, + createContainer, /** Simulates `container.wait()` resolving - the container process has exited. */ triggerContainerExit(statusCode = 0): void { resolveWait({ StatusCode: statusCode }); @@ -238,3 +255,312 @@ describe('DockerDriver.startRun - faithful demuxStream stub (regression: dockero expect(elapsedMs).toBeLessThan(1000); }); }); + +describe('DockerDriver.startRun - dev-folder mount composition (design.md: "Applying the mount")', () => { + it('adds exactly the bind + anonymous-volume Mounts entries when devMount is present, and never a Binds key', async () => { + const stub = stubDockerForRun(); + const driver = new DockerDriver(stub.docker); + driver.available = true; + + const outcomePromise = driver.startRun( + { + runId: 'run-mount-1', + imageId: 'fake-image', + env: {}, + memoryMbytes: 128, + timeoutSecs: 60, + devMount: { localDevFolder: '/host/src', imageWorkingDirectory: '/usr/src/app' }, + }, + () => {}, + ); + await new Promise((resolve) => setImmediate(resolve)); + + const [options] = stub.createContainer.mock.calls[0] as [{ HostConfig: Record }]; + expect(options.HostConfig.Mounts).toEqual([ + { Type: 'bind', Source: '/host/src', Target: '/usr/src/app' }, + { Type: 'volume', Source: '', Target: '/usr/src/app/node_modules' }, + ]); + expect(options.HostConfig.Binds).toBeUndefined(); + + stub.triggerContainerExit(0); + stub.endLogStream(); + await outcomePromise; + }); + + it('adds no Mounts key at all when devMount is absent (regression: unregistered Actors unaffected)', async () => { + const stub = stubDockerForRun(); + const driver = new DockerDriver(stub.docker); + driver.available = true; + + const outcomePromise = driver.startRun( + { runId: 'run-mount-2', imageId: 'fake-image', env: {}, memoryMbytes: 128, timeoutSecs: 60 }, + () => {}, + ); + await new Promise((resolve) => setImmediate(resolve)); + + const [options] = stub.createContainer.mock.calls[0] as [{ HostConfig: Record }]; + expect(options.HostConfig.Mounts).toBeUndefined(); + expect(options.HostConfig.Binds).toBeUndefined(); + + stub.triggerContainerExit(0); + stub.endLogStream(); + await outcomePromise; + }); + + it('logs an explicit mount line naming both the host and container paths, before the container is even created, when devMount is present', async () => { + const stub = stubDockerForRun(); + const driver = new DockerDriver(stub.docker); + driver.available = true; + const chunks: string[] = []; + + const outcomePromise = driver.startRun( + { + runId: 'run-mount-3', + imageId: 'fake-image', + env: {}, + memoryMbytes: 128, + timeoutSecs: 60, + devMount: { localDevFolder: '/host/src', imageWorkingDirectory: '/usr/src/app' }, + }, + (chunk) => chunks.push(chunk), + ); + await new Promise((resolve) => setImmediate(resolve)); + + expect(chunks.length).toBeGreaterThan(0); + expect(chunks[0]).toContain('/host/src'); + expect(chunks[0]).toContain('/usr/src/app'); + + stub.triggerContainerExit(0); + stub.endLogStream(); + await outcomePromise; + }); + + it('logs nothing extra when devMount is absent', async () => { + const stub = stubDockerForRun(); + const driver = new DockerDriver(stub.docker); + driver.available = true; + const chunks: string[] = []; + + const outcomePromise = driver.startRun( + { runId: 'run-mount-4', imageId: 'fake-image', env: {}, memoryMbytes: 128, timeoutSecs: 60 }, + (chunk) => chunks.push(chunk), + ); + await new Promise((resolve) => setImmediate(resolve)); + + expect(chunks).toEqual([]); + + stub.triggerContainerExit(0); + stub.endLogStream(); + await outcomePromise; + }); +}); + +describe('DockerDriver container removal passes { v: true } (design.md: "Volume cleanup is in this PR")', () => { + it("startRun's finally block removes the container with { v: true }, whether or not the run had a devMount", async () => { + const stub = stubDockerForRun(); + const driver = new DockerDriver(stub.docker); + driver.available = true; + + const outcomePromise = driver.startRun( + { runId: 'run-remove-1', imageId: 'fake-image', env: {}, memoryMbytes: 128, timeoutSecs: 60 }, + () => {}, + ); + await new Promise((resolve) => setImmediate(resolve)); + stub.triggerContainerExit(0); + stub.endLogStream(); + await outcomePromise; + + expect(stub.container.remove).toHaveBeenCalledWith({ v: true }); + }); +}); + +describe('DockerDriver.startBuild - imageWorkingDirectory capture (design.md: "Capture of imageWorkingDirectory")', () => { + /** A stub covering only what `startBuild` calls: `buildImage`, `modem.followProgress` (invoking its + * `onFinished` callback synchronously, as a successful build with no progress lines), and `getImage` + * for the post-build inspect. */ + function stubDockerForBuild(inspect: () => Promise<{ Config: { WorkingDir: string } }>) { + const followProgress = vi.fn( + ( + _stream: NodeJS.ReadableStream, + onFinished: (err: Error | null, res: Array<{ error?: string }>) => void, + ) => { + onFinished(null, []); + }, + ); + const getImage = vi.fn(() => ({ inspect })); + const docker = { + buildImage: vi.fn(async () => new PassThrough()), + modem: { followProgress }, + getImage, + } as unknown as Docker; + return { docker, getImage }; + } + + it("returns the image's Config.WorkingDir from docker.getImage(imageId).inspect(), never a shelled-out docker inspect", async () => { + const stub = stubDockerForBuild(async () => ({ Config: { WorkingDir: '/usr/src/app' } })); + const driver = new DockerDriver(stub.docker); + driver.available = true; + + const outcome = await driver.startBuild( + { buildId: 'build-1', actorName: 'my-actor', sourceFiles: [], useCache: true, timeoutSecs: 60 }, + () => {}, + ); + + expect(outcome.imageWorkingDirectory).toBe('/usr/src/app'); + expect(stub.getImage).toHaveBeenCalledWith(outcome.imageId); + }); + + it('tolerates an inspect rejection: the build still succeeds, with imageWorkingDirectory left unset', async () => { + const stub = stubDockerForBuild(async () => { + throw new Error('inspect failed'); + }); + const driver = new DockerDriver(stub.docker); + driver.available = true; + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + + const outcome = await driver.startBuild( + { buildId: 'build-2', actorName: 'my-actor', sourceFiles: [], useCache: true, timeoutSecs: 60 }, + () => {}, + ); + + expect(outcome.imageId).toBeTruthy(); + expect(outcome.imageWorkingDirectory).toBeUndefined(); + expect(warnSpy).toHaveBeenCalled(); + + warnSpy.mockRestore(); + }); + + it('leaves imageWorkingDirectory unset when the working directory is "/" (mounting over "/" would destroy the container)', async () => { + const stub = stubDockerForBuild(async () => ({ Config: { WorkingDir: '/' } })); + const driver = new DockerDriver(stub.docker); + driver.available = true; + + const outcome = await driver.startBuild( + { buildId: 'build-3', actorName: 'my-actor', sourceFiles: [], useCache: true, timeoutSecs: 60 }, + () => {}, + ); + + expect(outcome.imageWorkingDirectory).toBeUndefined(); + }); + + it('leaves imageWorkingDirectory unset when the working directory is empty', async () => { + const stub = stubDockerForBuild(async () => ({ Config: { WorkingDir: '' } })); + const driver = new DockerDriver(stub.docker); + driver.available = true; + + const outcome = await driver.startBuild( + { buildId: 'build-4', actorName: 'my-actor', sourceFiles: [], useCache: true, timeoutSecs: 60 }, + () => {}, + ); + + expect(outcome.imageWorkingDirectory).toBeUndefined(); + }); +}); + +describe('DockerDriver.probeDevFolder (design.md: "The check is a create-only probe container, never started")', () => { + it('returns ok and removes the (never-started) probe container on success, without ever calling .start()', async () => { + const start = vi.fn(); + const remove = vi.fn(async () => undefined); + const createContainer = vi.fn(async () => ({ remove, start })); + const driver = new DockerDriver({ createContainer } as unknown as Docker); + driver.available = true; + + const outcome = await driver.probeDevFolder('/abs/path', 'image:tag'); + + expect(outcome).toEqual({ ok: true }); + expect(createContainer).toHaveBeenCalledTimes(1); + const [options] = createContainer.mock.calls[0] as [{ Image: string; HostConfig: { Mounts: unknown[] } }]; + expect(options.Image).toBe('image:tag'); + expect(options.HostConfig.Mounts).toEqual([ + { Type: 'bind', Source: '/abs/path', Target: '/probe', ReadOnly: true }, + ]); + expect(remove).toHaveBeenCalledTimes(1); + expect(start).not.toHaveBeenCalled(); + }); + + it('never even calls createContainer when the driver already knows Docker is unavailable - short-circuits to unreachable', async () => { + const createContainer = vi.fn(async () => ({ remove: vi.fn() })); + const driver = new DockerDriver({ createContainer } as unknown as Docker); + // driver.available defaults to false - init() never ran. + + const outcome = await driver.probeDevFolder('/abs/path', 'image:tag'); + + expect(outcome).toEqual({ ok: false, reason: 'unreachable' }); + expect(createContainer).not.toHaveBeenCalled(); + }); + + it('classifies a rejection with no .statusCode as unreachable (a raw transport failure), never as "does not exist"', async () => { + const createContainer = vi.fn(async () => { + throw new Error('connect ECONNREFUSED /var/run/docker.sock'); + }); + const driver = new DockerDriver({ createContainer } as unknown as Docker); + driver.available = true; + + const outcome = await driver.probeDevFolder('/abs/path', 'image:tag'); + + expect(outcome).toEqual({ ok: false, reason: 'unreachable' }); + }); + + it("classifies a 404 rejection as image-missing (the probe's own image is gone, an operational fault)", async () => { + const createContainer = vi.fn(async () => { + throw Object.assign(new Error('(HTTP code 404) no such image: image:tag'), { statusCode: 404 }); + }); + const driver = new DockerDriver({ createContainer } as unknown as Docker); + driver.available = true; + + const outcome = await driver.probeDevFolder('/abs/path', 'image:tag'); + + expect(outcome).toEqual({ ok: false, reason: 'image-missing' }); + }); + + it('classifies the exact "bind source path does not exist" substring as not-found - the one case allowed to say so', async () => { + const createContainer = vi.fn(async () => { + throw Object.assign( + new Error( + '(HTTP code 400) client error - invalid mount config for type "bind": bind source path does not exist: /abs/path ', + ), + { statusCode: 400 }, + ); + }); + const driver = new DockerDriver({ createContainer } as unknown as Docker); + driver.available = true; + + const outcome = await driver.probeDevFolder('/abs/path', 'image:tag'); + + expect(outcome).toEqual({ ok: false, reason: 'not-found' }); + }); + + it('classifies "source path must be a directory" as unknown, never as not-found', async () => { + const createContainer = vi.fn(async () => { + throw Object.assign( + new Error( + '(HTTP code 400) client error - invalid mount config for type "bind": source path must be a directory', + ), + { statusCode: 400 }, + ); + }); + const driver = new DockerDriver({ createContainer } as unknown as Docker); + driver.available = true; + + const outcome = await driver.probeDevFolder('/abs/path', 'image:tag'); + + expect(outcome).toEqual({ ok: false, reason: 'unknown' }); + }); + + it('classifies a Docker Desktop file-sharing denial (a real, existing path) as unknown, never as not-found - the false-negative this design deliberately avoids', async () => { + const createContainer = vi.fn(async () => { + throw Object.assign( + new Error( + '(HTTP code 400) client error - Mounts denied: The path /abs/path is not shared from the host and is not known to Docker.', + ), + { statusCode: 400 }, + ); + }); + const driver = new DockerDriver({ createContainer } as unknown as Docker); + driver.available = true; + + const outcome = await driver.probeDevFolder('/abs/path', 'image:tag'); + + expect(outcome).toEqual({ ok: false, reason: 'unknown' }); + }); +}); From 04ab19671117e38abe0bddf17a8dbc109b5823be Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 19 Aug 2026 07:45:17 +0000 Subject: [PATCH 02/11] Address review: drop dangling doc citations, share build resolution Code comments cited files that are gitignored, so they were dead pointers for anyone reading the repository. They now cite moby/dockerode behaviour in its own terms or the committed requirements sections instead. The reasoning is unchanged; only the references are. Probe image resolution moves onto a shared resolveTaggedBuild helper, also now used by the run-start path it was duplicating. This drops an arbitrary-tag fallback: an Actor whose only build is not tagged latest is refused at registration, matching what a tag-less run already does rather than succeeding against a build no run could reach. Adds coverage for the build-time working-directory persist, both when the build reports one and when it does not and a stored value must survive. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_011L4VcFqN9UZUbVRMvQSugv --- src/api/routes/actors.ts | 8 +-- src/api/routes/dev-folder.ts | 20 +++--- src/console/server.ts | 34 +++++----- src/driver/docker-driver.ts | 78 ++++++++++++----------- src/driver/types.ts | 20 +++--- src/services/actors.ts | 84 +++++++++++++++---------- src/services/builds.ts | 20 +++--- src/services/runs.ts | 8 +-- src/storage/entities.ts | 8 ++- test/e2e/dev-folder-bind-mount.test.ts | 33 +++++----- test/integration/dev-folder.test.ts | 25 +++++++- test/integration/job-lifecycle.test.ts | 65 +++++++++++++++++++ test/unit/dev-folder-validation.test.ts | 5 +- test/unit/docker-driver.test.ts | 10 +-- 14 files changed, 265 insertions(+), 153 deletions(-) diff --git a/src/api/routes/actors.ts b/src/api/routes/actors.ts index d2083b3..561eabb 100644 --- a/src/api/routes/actors.ts +++ b/src/api/routes/actors.ts @@ -12,6 +12,7 @@ import { findVersion, listOwnedActors, resolveOwnedActor, + resolveTaggedBuild, updateActor, } from '../../services/actors.js'; import { listOwnedBuilds, startBuild, waitForBuildFinish, type StartBuildOptions } from '../../services/builds.js'; @@ -274,11 +275,8 @@ export function mountActors(router: Router, deps: ApiServerDeps): void { if (!actor) throw recordNotFound(); const tag = queryString(req, 'build') ?? DEFAULT_TAG; - const tagged = actor.taggedBuilds[tag]; - if (!tagged) throw recordNotFound(`Actor has no build tagged "${tag}"`); - const { builds } = getRegistries(); - const build = await builds.get(tagged.buildId); - if (!build) throw recordNotFound(); + const build = await resolveTaggedBuild(actor, tag); + if (!build) throw recordNotFound(`Actor has no build tagged "${tag}"`); const body = rawBody(req); const input = diff --git a/src/api/routes/dev-folder.ts b/src/api/routes/dev-folder.ts index 1dad145..74db10e 100644 --- a/src/api/routes/dev-folder.ts +++ b/src/api/routes/dev-folder.ts @@ -1,14 +1,14 @@ /** * `POST /actor-runtime/dev-folder/:actorId` - deliberately outside the emulated `/v2` surface - * (`api.md`'s `/actor-runtime/*` namespace; `design.md`'s Decisions #1/#8), so this is mounted directly - * on the API `app`, not the `v2` router (`server.ts`'s "Auth is per-router, not global" note) - it - * therefore needs its own `auth()`, applied to a small router of its own below, not inherited from `v2`. + * (`api.md`'s `/actor-runtime/*` namespace), so this is mounted directly on the API `app`, not the `v2` + * router (`server.ts`'s "Auth is per-router, not global" note) - it therefore needs its own `auth()`, + * applied to a small router of its own below, not inherited from `v2`. * - * Canonical body is a JSON string: `'"/abs/path"'` to set, `'""'` to clear (`design.md`'s Decision #6 - - * `apify api`'s own `--body` validates with `JSON.parse` and refuses anything that isn't valid JSON, so - * a bare, unquoted path can never reach this route through the documented CLI invocation at all). A - * JSON value that parses but isn't a string (a number, an object, ...) is rejected the same way a - * malformed body is - only a genuine JSON string is ever a valid registration payload. + * Canonical body is a JSON string: `'"/abs/path"'` to set, `'""'` to clear (`api.md`'s `/actor-runtime/*` + * section - `apify api`'s own `--body` validates with `JSON.parse` and refuses anything that isn't + * valid JSON, so a bare, unquoted path can never reach this route through the documented CLI invocation + * at all). A JSON value that parses but isn't a string (a number, an object, ...) is rejected the same + * way a malformed body is - only a genuine JSON string is ever a valid registration payload. * * Ownership-scoped like every other Actor write on this API port: `resolveOwnedActor` (not the * console's cross-user `getActorById`) so a caller can only ever register a dev folder for their own @@ -50,8 +50,8 @@ export function mountDevFolder(app: Express, deps: ApiServerDeps): void { throw new ApiError(info.status, info.type, info.message); } - // The response body doubles as the read-back this design deliberately has no separate `GET` - // for yet (`design.md`'s Follow-ups) - the same three fields the console detail page shows. + // The response body doubles as the read-back - there is deliberately no separate `GET` for + // this yet - with the same three fields the console detail page shows. sendData(res, devFolderStatus(result.actor)); }), ); diff --git a/src/console/server.ts b/src/console/server.ts index 55199b9..043f60a 100644 --- a/src/console/server.ts +++ b/src/console/server.ts @@ -5,13 +5,14 @@ * shared rather than reimplemented. * * The console itself has no login of its own - it is unauthenticated, and view-only except for exactly - * one mutation (`console.md`, amended by `design.md`'s Decision #3): the dev-folder form on the Actor - * detail view. With multiple users it does not scope reads to any one of them: every list/detail route - * below reads through the `listAll*`/`get*ById` cross-user service functions (see e.g. - * `services/actors.ts: listAllActors`), never the API's own per-user `listOwned*`/`getOwned*`, and every - * list row and detail view shows the object's owner `userId` (`console.md`: "Frontend shows for each - * object the owner (userId)"). The dev-folder form writes cross-user the same way - a documented - * deviation from the API's own strictly-owner-scoped write, not an accident (`design.md`'s Risks). + * one mutation (`console.md`'s "The console is unauthenticated, and view-only except for exactly one + * mutation"): the dev-folder form on the Actor detail view. With multiple users it does not scope reads + * to any one of them: every list/detail route below reads through the `listAll*`/`get*ById` cross-user + * service functions (see e.g. `services/actors.ts: listAllActors`), never the API's own per-user + * `listOwned*`/`getOwned*`, and every list row and detail view shows the object's owner `userId` + * (`console.md`: "Frontend shows for each object the owner (userId)"). The dev-folder form writes + * cross-user the same way - a documented deviation from the API's own strictly-owner-scoped write, not + * an accident (`console.md`'s dev-folder section again: "a documented deviation... not an accident"). */ import express, { type Express } from 'express'; @@ -44,8 +45,9 @@ export interface ConsoleServerDeps { } /** The dev-folder registration form + its three read-only status rows, rendered on the Actor detail - * view (`design.md`'s console decisions). `errorMessage` is threaded through from the POST handler's - * redirect query param below, since a redirect itself carries no state of its own. */ + * view (`console.md`'s "Local dev-folder registration form" section). `errorMessage` is threaded + * through from the POST handler's redirect query param below, since a redirect itself carries no state + * of its own. */ function devFolderSection(actorId: string, status: ReturnType, errorMessage?: string): string { const errorHtml = errorMessage ? `

Error: ${escapeHtml(errorMessage)}

` @@ -126,13 +128,13 @@ export function createConsoleServer(deps: ConsoleServerDeps): Express { }); /** - * The console's one mutation (`design.md`'s Decision #3) - funnels through the exact same - * `setDevFolder` the API's `POST /actor-runtime/dev-folder/:actorId` uses, resolving the Actor - * cross-user by the id already in the page URL (no token, matching the console's existing - * unauthenticated reads) rather than through `resolveOwnedActor`. A failure redirects back with the - * classified message in a query param - `describeDevFolderError`'s wording, not a bespoke one - so - * the build-first rejection and the does-not-exist/could-not-verify distinction are surfaced, not - * swallowed (success criterion 27). + * The console's one mutation (`console.md`'s "Local dev-folder registration form" section) - + * funnels through the exact same `setDevFolder` the API's `POST /actor-runtime/dev-folder/:actorId` + * uses, resolving the Actor cross-user by the id already in the page URL (no token, matching the + * console's existing unauthenticated reads) rather than through `resolveOwnedActor`. A failure + * redirects back with the classified message in a query param - `describeDevFolderError`'s wording, + * not a bespoke one - so the build-first rejection and the does-not-exist/could-not-verify + * distinction are surfaced, not swallowed. */ app.post('/actors/:id/dev-folder', async (req, res) => { const actor = await getActorById(req.params.id); diff --git a/src/driver/docker-driver.ts b/src/driver/docker-driver.ts index 4e9cf5d..6292aae 100644 --- a/src/driver/docker-driver.ts +++ b/src/driver/docker-driver.ts @@ -40,13 +40,12 @@ const NETWORK_NAME = 'apify-local'; const RUN_LABEL = 'actor-runtime.runId'; /** Target path for the create-only, never-started existence probe container (`probeDevFolder` below) - * arbitrary, since the probe is never started and nothing ever reads from it; moby validates the mount - * source before the container object is even returned (`_request_fact_check.md`'s round-3 delta, - * claim 2). */ + * source before the container object is even returned from `POST /containers/create`, so no path is + * ever read from this target. */ const PROBE_MOUNT_TARGET = '/probe'; /** The daemon's own fixed error-message substring for a `Mounts`-type bind whose source is missing - * (moby's `daemon/volume/mounts/validate.go: errBindSourceDoesNotExist`, pinned by - * `_request_fact_check.md`'s round-3 delta, claim 1) - the one rejection shape `classifyProbeError` - * reports as "does not exist" rather than a generic "could not verify". */ + * (moby's `daemon/volume/mounts/validate.go: errBindSourceDoesNotExist`) - the one rejection shape + * `classifyProbeError` reports as "does not exist" rather than a generic "could not verify". */ const BIND_SOURCE_MISSING_SUBSTRING = 'bind source path does not exist'; /** Narrows an unknown rejection to the shape `docker-modem` attaches to a daemon HTTP-level error @@ -228,10 +227,10 @@ export class DockerDriver implements Driver { } /** - * `.Config.WorkingDir` of the image just built, via `docker.getImage(imageId).inspect()` - * (`design.md`'s "Capture of `imageWorkingDirectory`" - this codebase talks to the host socket - * through `dockerode` only, never a shelled-out `docker inspect`, correcting - * `actor-driver.md`'s original CLI-flag phrasing). An inspect failure is logged and tolerated - it + * `.Config.WorkingDir` of the image just built, via `docker.getImage(imageId).inspect()` - this + * codebase talks to the host socket through `dockerode` exclusively, never a shelled-out + * `docker inspect` (see `actor-driver.md`'s "Bind mount volumes with Actor source code" section, and + * this file's own class doc comment above). An inspect failure is logged and tolerated - it * must never fail an otherwise-successful build - and an empty or `/` working directory is treated * the same as "unknown": mounting a dev folder over `/` at run start would destroy the container. */ @@ -263,11 +262,12 @@ export class DockerDriver implements Driver { const env = Object.entries(ctx.env).map(([key, value]) => `${key}=${value}`); - // Observability of the mount (`design.md`): a secondary diagnostic now that existence is verified - // at registration - if the folder is deleted/moved/made unreadable between registration and this - // run, the daemon's own rejection below explains why the run failed, but only if the very first - // log line already named the two paths. Written before `createContainer` so it is genuinely first, - // even if the daemon call itself is what ends up failing. + // Observability of the mount (`actor-driver.md`'s "Observability" bullet): a secondary + // diagnostic now that existence is verified at registration - if the folder is deleted/moved/made + // unreadable between registration and this run, the daemon's own rejection below explains why the + // run failed, but only if the very first log line already named the two paths. Written before + // `createContainer` so it is genuinely first, even if the daemon call itself is what ends up + // failing. if (ctx.devMount) { onLog( `Mounting local dev folder ${ctx.devMount.localDevFolder} over the image's working directory ` + @@ -367,25 +367,26 @@ export class DockerDriver implements Driver { this.timedOutRuns.delete(ctx.runId); this.runContainers.delete(ctx.runId); // `{ v: true }` also removes the container's anonymous volumes - without it, the anonymous - // `node_modules` volume `buildDevMounts` adds for a `devMount` run would leak one volume per run, - // forever (`design.md`'s "Volume cleanup is in this PR"). Harmless for a run with no `devMount`: - // such a container has no anonymous volumes to remove in the first place. + // `node_modules` volume `buildDevMounts` adds for a `devMount` run would leak one volume per + // run, forever (see `actor-driver.md`'s "Every run's container removal passes `{ v: true }`" + // bullet). Harmless for a run with no `devMount`: such a container has no anonymous volumes to + // remove in the first place. await container.remove({ v: true }).catch(() => undefined); } } /** - * The two `HostConfig.Mounts` entries for a `devMount` run (`design.md`'s "Applying the mount") - - * `Mounts`, not `Binds`, for the same reason `probeDevFolder` uses `Mounts`: a `Mounts`-type bind - * errors on a missing source instead of silently auto-creating one (`_request_fact_check.md`'s - * round-3 delta, claim 1), so a folder that vanished between registration and this run start fails - * the run loudly instead of masking the image's own working directory with an empty auto-created - * directory. The bind is read-write (no `ReadOnly`), matching the requirement's own plain `-v` form. - * The second entry - `Type: 'volume'` with an empty `Source` - is the `Mounts`-array equivalent of the - * anonymous-volume bare-path `-v` form: Docker copies the image's existing `node_modules` into it - * before mounting, which is what preserves the image's installed dependencies underneath a bind that - * otherwise covers the whole working directory (a *named* volume would start empty; a plain bind - * would erase - `_request_fact_check.md`'s claim 6). + * The two `HostConfig.Mounts` entries for a `devMount` run (`actor-driver.md`'s "The mount uses + * `HostConfig.Mounts`..." bullet) - `Mounts`, not `Binds`, for the same reason `probeDevFolder` uses + * `Mounts`: a `Mounts`-type bind errors on a missing source instead of silently auto-creating one + * (moby's `daemon/volume/mounts/validate.go`, unlike the legacy `Binds`/`-v` auto-create behavior), so + * a folder that vanished between registration and this run start fails the run loudly instead of + * masking the image's own working directory with an empty auto-created directory. The bind is + * read-write (no `ReadOnly`), matching the requirement's own plain `-v` form. The second entry - + * `Type: 'volume'` with an empty `Source` - is the `Mounts`-array equivalent of the anonymous-volume + * bare-path `-v` form: Docker copies the image's existing `node_modules` into it before mounting, + * which is what preserves the image's installed dependencies underneath a bind that otherwise covers + * the whole working directory (a *named* volume would start empty; a plain bind would erase it). */ private buildDevMounts(devMount: DevFolderMount): Docker.MountSettings[] { return [ @@ -395,21 +396,22 @@ export class DockerDriver implements Driver { } /** - * Host-side existence check for a candidate dev-folder path (`design.md`'s "Registration"): a - * create-only probe container, never started. `fs.existsSync` would test this *runtime process's* - * filesystem, not the host's (this driver always runs against the host's own Docker socket - see the - * class doc comment); the only Engine API surface that validates an arbitrary host path at all is the - * mount-validation moby runs inside `POST /containers/create` (`_request_fact_check.md`'s round-3 - * delta, claims 2 and 4). `BindOptions.CreateMountpoint` (the option that would auto-create a missing - * source and defeat this check entirely) is deliberately never set - `@types/dockerode`'s own + * Host-side existence check for a candidate dev-folder path (`actor-driver.md`'s "Registration + * validates the path in two layers" bullet): a create-only probe container, never started. + * `fs.existsSync` would test this *runtime process's* filesystem, not the host's (this driver always + * runs against the host's own Docker socket - see the class doc comment); the only Engine API + * surface that validates an arbitrary host path at all is the mount-validation moby runs inside + * `POST /containers/create`. `BindOptions.CreateMountpoint` (the option that would auto-create a + * missing source and defeat this check entirely) is deliberately never set - `@types/dockerode`'s own * `BindOptions` type doesn't even declare it, so the straightforward, type-safe object literal below * omits it for free. On success the probe is removed immediately without ever being started; on * rejection there is nothing to clean up, since creation itself is what failed. * * `imageId` is always the Actor's own latest successfully-built image (resolved by - * `services/actors.ts: setDevFolder`), never a self-inspected runtime image or a pulled one - see - * `design.md`'s rejected alternative on self-inspection via `HOSTNAME`, which `selfAttachToNetwork` - * above already documents as unset in bare local dev, exactly where this feature is used. + * `services/actors.ts: setDevFolder`), never a self-inspected runtime image or a pulled one: a + * self-inspected runtime image was rejected as the probe/mount image because `HOSTNAME` is unset in + * bare local dev, which `selfAttachToNetwork` above already documents - exactly the environment this + * feature targets - and a pulled image would break the offline-after-first-build property. */ async probeDevFolder(candidatePath: string, imageId: string): Promise { // Known-unavailable short-circuits without ever touching the socket - the same outcome diff --git a/src/driver/types.ts b/src/driver/types.ts index 45d6915..a96d9fe 100644 --- a/src/driver/types.ts +++ b/src/driver/types.ts @@ -10,8 +10,9 @@ export interface BuildContext { /** * Host folder + image working directory, carried together so "both or neither" is enforced by the - * type itself (`design.md`'s "Applying the mount") - there is no way to construct a `RunContext` with - * one field set and the other missing. `services/runs.ts` builds this only when the Actor's own + * type itself - there is no way to construct a `RunContext` with one field set and the other missing, + * matching `actor-driver.md`'s "The mount is conditional, applied only when both fields are present and + * non-empty". `services/runs.ts` builds this only when the Actor's own * `localDevFolder`/`imageWorkingDirectory` are both present and non-empty; `docker-driver.ts`'s * `startRun` adds the `HostConfig.Mounts` entries only when this is present at all. */ @@ -32,16 +33,18 @@ export interface RunContext { export interface BuildOutcome { imageId: string; /** `.Config.WorkingDir` of the image `startBuild` just built, captured via - * `docker.getImage(imageId).inspect()` (`design.md`: this codebase talks to the host socket through - * dockerode only, never a shelled-out `docker inspect`). Unset when the inspect call itself failed + * `docker.getImage(imageId).inspect()` - this codebase talks to the host socket through dockerode + * only, never a shelled-out `docker inspect` (`actor-driver.md`'s "`imageWorkingDirectory` is + * captured by the driver itself" bullet). Unset when the inspect call itself failed * (logged, never fails the build) or when the working directory was empty/`/` (mounting over `/` * would destroy the container). */ imageWorkingDirectory?: string; } /** - * Why a candidate dev-folder path was rejected by the host-side existence probe (`design.md`'s - * "Registration" section), classified by error shape, most specific first: + * Why a candidate dev-folder path was rejected by the host-side existence probe (`actor-driver.md`'s + * "Registration validates the path in two layers" bullet), classified by error shape, most specific + * first: * - `unreachable`: no HTTP response at all (raw socket error, or the driver already knows Docker is * unavailable) - never asserted as "does not exist". * - `image-missing`: the probe's own image (the Actor's latest successfully-built image) returned 404 @@ -102,8 +105,9 @@ export interface Driver { reconcileOrphans(runIds: string[]): Promise; /** - * Host-side existence probe for a candidate dev-folder path (`design.md`'s "Registration"), used - * only by `services/actors.ts: setDevFolder` - never by the build/run lifecycle. Deliberately + * Host-side existence probe for a candidate dev-folder path (`actor-driver.md`'s "Registration + * validates the path in two layers" bullet), used only by `services/actors.ts: setDevFolder` - + * never by the build/run lifecycle. Deliberately * **optional**: every pre-existing stub `Driver` throughout the test suite (none of which model a * real dockerode handle - `test/integration/helpers/test-server.ts` and several integration test * files construct `Driver` literals directly) keeps compiling unchanged, since only `DockerDriver` diff --git a/src/services/actors.ts b/src/services/actors.ts index 2591c05..42a2027 100644 --- a/src/services/actors.ts +++ b/src/services/actors.ts @@ -1,8 +1,27 @@ import { generateId } from '../storage/ids.js'; -import type { ActorRecord, ActorVersionRecord } from '../storage/entities.js'; +import type { ActorRecord, ActorVersionRecord, BuildRecord } from '../storage/entities.js'; import { getRegistries } from '../storage/registries.js'; import type { Driver, DevFolderProbeOutcome } from '../driver/types.js'; +/** The tag a build/run resolves to when the caller names none - mirrored by `POST + * /actors/:actorId/runs` (`api/routes/actors.ts`'s own `DEFAULT_TAG`) and by `resolveDevFolderProbeBuild` + * below, which shares this constant so the two can never drift apart. */ +export const DEFAULT_BUILD_TAG = 'latest'; + +/** + * Resolves the `BuildRecord` tagged `tag` on this Actor - the exact lookup `POST + * /actors/:actorId/runs` performs (`actor.taggedBuilds[tag]`, then load that build by id), extracted + * here so the registration probe below can resolve an image the identical way a real run does, instead + * of re-implementing it. A missing tag and a missing build record are both reported the same way a + * missing tag is at that route: `null`, with no fallback to any other tag. + */ +export async function resolveTaggedBuild(actor: ActorRecord, tag: string): Promise { + const tagged = actor.taggedBuilds[tag]; + if (!tagged) return null; + const build = await getRegistries().builds.get(tagged.buildId); + return build ?? null; +} + export interface CreateActorInput { name: string; title?: string; @@ -108,7 +127,7 @@ export function recordTaggedBuild(actor: ActorRecord, tag: string, buildId: stri return { ...actor, taggedBuilds: { ...actor.taggedBuilds, [tag]: { buildId, buildNumber } } }; } -// --- Local dev-folder registration (`design.md`) --- +// --- Local dev-folder registration (`actor-driver.md`'s "Bind mount volumes with Actor source code") --- // // One validate-and-persist entry point, `setDevFolder`, shared by the API's // `POST /actor-runtime/dev-folder/:actorId` (`api/routes/dev-folder.ts`) and the console's single-field @@ -116,8 +135,8 @@ export function recordTaggedBuild(actor: ActorRecord, tag: string, buildId: stri // callers pass an already-unwrapped, already-trimmed string: the API unwraps its JSON-string body, the // console reads its urlencoded form field. -/** Cheap shape pre-filter, run before the host-side existence check, never instead of it (`design.md`'s - * "Validation is now two layered checks, not shape alone"). `~` is not expanded - this codebase never +/** Cheap shape pre-filter, run before the host-side existence check, never instead of it (see + * `actor-driver.md`'s "Registration validates the path in two layers" bullet). `~` is not expanded - this codebase never * shells out (see `docker-driver.ts`'s class doc comment), so there is no shell to expand it, and * expanding it here would require guessing which host user's home directory this runtime process * should assume. Returns `null` for a shape-valid non-empty path, or a human-readable rejection reason. @@ -139,23 +158,19 @@ export function validateDevFolderPathShape(path: string): string | null { } /** - * Resolves the image id `setDevFolder` hands to `driver.probeDevFolder` - the Actor's own latest - * successfully-built image, "the same id the driver already uses to start real runs" (`design.md`). - * `taggedBuilds` is only ever populated by `recordTaggedBuild`, itself only called after a build - * transitions to `SUCCEEDED` (`services/builds.ts: runBuildInBackground`), so any entry at all is proof - * of a genuine past success - matching how `POST /actors/:actorId/runs` resolves a run's build by tag - * (`api/routes/actors.ts`'s `DEFAULT_TAG`), this prefers the `latest` tag when present and otherwise - * falls back to whichever tag exists. Returns `null` when the Actor has never had a successful build at - * all - the build-first precondition (`design.md`'s Decisions #9-adjacent scope-split; success - * criterion 6). + * Resolves the image id `setDevFolder` hands to `driver.probeDevFolder` - via `resolveTaggedBuild` at + * `DEFAULT_BUILD_TAG` ('latest'), the exact same resolution `POST /actors/:actorId/runs` performs when + * a run's caller names no `?build=` tag (`api/routes/actors.ts`'s own default). There is deliberately + * no fallback to some other tag when `latest` is absent: an Actor whose only successful build(s) are + * tagged something else is exactly the Actor a tag-less real run would also 404 against, so the probe + * must refuse it the same way, not silently succeed against a build a real run could not reach without + * an explicit tag. `taggedBuilds` is only ever populated by `recordTaggedBuild`, itself only called + * after a build transitions to `SUCCEEDED` (`services/builds.ts: runBuildInBackground`), so a resolved + * build is always proof of a genuine past success. Returns `null` when the Actor has never had a + * `latest`-tagged successful build - the build-first precondition. */ async function resolveProbeImageId(actor: ActorRecord): Promise { - const tags = Object.keys(actor.taggedBuilds); - const preferredTag = actor.taggedBuilds.latest ? 'latest' : tags[0]; - if (!preferredTag) return null; - const tagged = actor.taggedBuilds[preferredTag]; - if (!tagged) return null; - const build = await getRegistries().builds.get(tagged.buildId); + const build = await resolveTaggedBuild(actor, DEFAULT_BUILD_TAG); return build?.imageId ?? null; } @@ -172,17 +187,17 @@ export type SetDevFolderResult = | { kind: 'unknown' }; /** - * The one validate-and-persist path both the API endpoint and the console form funnel through - * (`design.md`: "Both paths funnel into one service function that validates and persists"). `path` is - * already unwrapped from its transport encoding and trimmed by the caller. + * The one validate-and-persist path both the API endpoint and the console form funnel through - see + * this file's own module comment above ("Local dev-folder registration"). `path` is already unwrapped + * from its transport encoding and trimmed by the caller. * * An empty `path` is a first-class "clear" operation - it always succeeds, never runs the shape check, - * the build-first check, or the existence probe (`design.md`: "Clearing... never runs the existence - * check, since there is no path to check"). A non-empty `path` must pass the shape pre-filter, then - * requires the Actor to have at least one successful build (so there is an image to probe against at - * all), then must pass the host-side existence probe - in that order, each one short-circuiting the - * next on failure. A rejected call never touches `updateActor` at all, so a previously-registered value - * survives untouched across a later failed registration attempt (success criterion 8). + * the build-first check, or the existence probe (`actor-driver.md`: "Submitting the empty string clears + * the registration and never runs either validation layer"). A non-empty `path` must pass the shape + * pre-filter, then requires the Actor to have at least one successful build (so there is an image to + * probe against at all), then must pass the host-side existence probe - in that order, each one + * short-circuiting the next on failure. A rejected call never touches `updateActor` at all, so a + * previously-registered value survives untouched across a later failed registration attempt. */ export async function setDevFolder(driver: Driver, actor: ActorRecord, path: string): Promise { if (path === '') { @@ -212,8 +227,9 @@ export interface DevFolderErrorInfo { } /** Maps every non-`ok` `SetDevFolderResult` to the status/type/message the API route wraps in an - * `ApiError` and the console form renders inline - one mapping, two presentations (`design.md`'s error - * classification, most specific first: unreachable/image-missing/not-found/unknown). */ + * `ApiError` and the console form renders inline - one mapping, two presentations of the error + * classification `actor-driver.md` documents (most specific first: unreachable/image-missing/ + * not-found/unknown). */ export function describeDevFolderError(result: Exclude): DevFolderErrorInfo { switch (result.kind) { case 'invalid-path': @@ -255,14 +271,14 @@ export interface DevFolderStatus { localDevFolder: string | null; imageWorkingDirectory: string | null; /** Whether `startRun` will actually add the bind mount on this Actor's next run - `true` only when - * both fields are present and non-empty (`design.md`: "No mount is added when either field is - * missing or the folder is empty"). Shown separately from the two raw fields so the console/API - * caller never has to re-derive this condition themselves (success criterion 28). */ + * both fields are present and non-empty (`actor-driver.md`: "The mount is conditional, applied only + * when both fields are present and non-empty"). Shown separately from the two raw fields so the + * console/API caller never has to re-derive this condition themselves. */ mountWillApply: boolean; } /** The three values both the API's registration response and the console detail page show - one - * derivation, so they can never drift apart (success criterion 27). */ + * derivation, so they can never drift apart. */ export function devFolderStatus(actor: ActorRecord): DevFolderStatus { const localDevFolder = actor.localDevFolder ?? null; const imageWorkingDirectory = actor.imageWorkingDirectory ?? null; diff --git a/src/services/builds.ts b/src/services/builds.ts index cf7e825..b3aa6d7 100644 --- a/src/services/builds.ts +++ b/src/services/builds.ts @@ -192,15 +192,17 @@ export async function runBuildInBackground( // `apify call`/`POST .../runs` against that tag even though the build record itself correctly // stayed ABORTED. if (succeeded?.status === 'SUCCEEDED') { - // Folded into the same `updateActor` call that records the tagged build (`design.md`'s - // "Capture of `imageWorkingDirectory`"), so it lands in `__ACTORS__` in one write, not two. Only - // set when this build's inspect actually produced a value: `outcome.imageWorkingDirectory` is - // `undefined` both when the inspect call itself failed and when the image's working directory was - // empty/`/` (`docker-driver.ts`'s `inspectWorkingDirectory`) - either way, that must never fail the - // (otherwise-successful) build, and it must not clobber a previously known-good value with - // `undefined` just because *this* build's inspect happened to come up empty (`design.md`'s "Stale - // working directory" risk already accepts the field reflecting an older build; overwriting a known - // value with "unset" on a transient inspect hiccup would be strictly worse than that, not better). + // Folded into the same `updateActor` call that records the tagged build (`actor-driver.md`'s + // "`imageWorkingDirectory` is captured by the driver itself" bullet), so it lands in + // `__ACTORS__` in one write, not two. Only set when this build's inspect actually produced a + // value: `outcome.imageWorkingDirectory` is `undefined` both when the inspect call itself + // failed and when the image's working directory was empty/`/` (`docker-driver.ts`'s + // `inspectWorkingDirectory`) - either way, that must never fail the (otherwise-successful) + // build, and it must not clobber a previously known-good value with `undefined` just because + // *this* build's inspect happened to come up empty (`storage.md`'s `imageWorkingDirectory` + // field already accepts reflecting only the most recent successful build, a known staleness + // gap; overwriting a known value with "unset" on a transient inspect hiccup would be strictly + // worse than that, not better). await updateActor(actor.id, (current) => { const withTag = recordTaggedBuild(current, options.tag, record.id, record.buildNumber); return outcome.imageWorkingDirectory !== undefined diff --git a/src/services/runs.ts b/src/services/runs.ts index 6684702..9c90f09 100644 --- a/src/services/runs.ts +++ b/src/services/runs.ts @@ -209,10 +209,10 @@ export async function runInBackground( const env = buildEnv(record, actor, version, options); // Both-or-neither, enforced by `DevFolderMount`'s type (`driver/types.ts`) - a mount is only ever // added when the Actor actually has a non-empty registered dev folder AND a known, non-empty image - // working directory (`design.md`: "No mount is added when either field is missing or the folder is - // empty"). An Actor that was never registered (or was cleared) gets `devMount: undefined`, which - // `docker-driver.ts`'s `startRun` treats identically to "no `Mounts` key at all" - the regression - // guarantee (success criterion 23). + // working directory (`actor-driver.md`: "The mount is conditional, applied only when both fields are + // present and non-empty"). An Actor that was never registered (or was cleared) gets `devMount: + // undefined`, which `docker-driver.ts`'s `startRun` treats identically to "no `Mounts` key at all" - + // the regression guarantee that an unregistered/cleared Actor's run container is unaffected. const devMount = actor.localDevFolder && actor.imageWorkingDirectory ? { localDevFolder: actor.localDevFolder, imageWorkingDirectory: actor.imageWorkingDirectory } diff --git a/src/storage/entities.ts b/src/storage/entities.ts index 3af507f..b9f62f2 100644 --- a/src/storage/entities.ts +++ b/src/storage/entities.ts @@ -77,9 +77,11 @@ export interface ActorRecord { * right after that build (`docker-driver.ts`'s `startBuild`) and persisted in the same `updateActor` * call that records the tagged build (`services/builds.ts`). Optional: unset until at least one build * has succeeded and its image could be inspected, and left unset (not overwritten) by a build whose - * inspect failed or whose image's working directory was empty/`/` (`design.md`: mounting over `/` - * would destroy the container). Reflects the *most recent* successful build only - see `design.md`'s - * "Stale working directory" risk. Optional and never exposed on `/v2`, same as `localDevFolder`. + * inspect failed or whose image's working directory was empty/`/` (mounting over `/` would destroy + * the container). Reflects the *most recent* successful build only - running an older, differently + * tagged build whose image had a different working directory is a known staleness gap, accepted for + * the POC (`actor-driver.md`'s "`imageWorkingDirectory` is captured by the driver itself" bullet). + * Optional and never exposed on `/v2`, same as `localDevFolder`. */ imageWorkingDirectory?: string; } diff --git a/test/e2e/dev-folder-bind-mount.test.ts b/test/e2e/dev-folder-bind-mount.test.ts index edb089a..5a85dac 100644 --- a/test/e2e/dev-folder-bind-mount.test.ts +++ b/test/e2e/dev-folder-bind-mount.test.ts @@ -1,7 +1,7 @@ /** - * E2E case for the local dev-folder bind mount (`design.md`'s Testability section, "Only the real-Docker - * e2e suite can prove a genuine host path passes the probe and the mount itself"): after one real - * push+build, registering the Actor's host source folder via the documented + * E2E case for the local dev-folder bind mount - only a real Docker daemon can prove a genuine host + * path passes the probe and the mount itself, so this is the one place that exercise runs: after one + * real push+build, registering the Actor's host source folder via the documented * `apify api POST ../actor-runtime/dev-folder/` invocation and then recompiling *locally* must * be picked up by the *next* `apify call`, with no intervening `apify push`/build - and the image's own * `node_modules` must survive the mount (the anonymous-volume guarantee). Registration is itself an @@ -40,11 +40,11 @@ const ACTOR_DIR = join(REPO_ROOT, 'sample_actor_ts'); const MAIN_TS = join(ACTOR_DIR, 'src', 'main.ts'); const CONTAINER_NAME = 'actor-runtime-e2e-devfolder'; const IMAGE_TAG = 'actor-runtime:e2e-devfolder'; -// `sample_actor_ts/Dockerfile` sets no `WORKDIR` of its own, so it inherits the base image's -// (`apify/actor-node`'s own Dockerfile - confirmed live against its current default branch in -// `.shepherd/_request_fact_check.md`'s round-1 claim 7). Asserted independently below via the -// registration response's own `imageWorkingDirectory`, not only assumed here - if the base image ever -// moves its `WORKDIR`, that assertion (not the mount itself) is what will fail first and explain why. +// `sample_actor_ts/Dockerfile` sets no `WORKDIR` of its own, so it inherits the base image's - the +// `apify/actor-node` image's own Dockerfile sets `WORKDIR /usr/src/app`. Asserted independently below +// via the registration response's own `imageWorkingDirectory`, not only assumed here - if the base +// image ever moves its `WORKDIR`, that assertion (not the mount itself) is what will fail first and +// explain why. const EXPECTED_IMAGE_WORKING_DIR = '/usr/src/app'; const ORIGINAL_MARKER = 'Crawl finished.'; const EDITED_MARKER = 'Crawl finished (dev-folder-edit-marker).'; @@ -56,10 +56,10 @@ interface DevFolderApiResult { } function registerDevFolder(actorId: string, path: string, env: NodeJS.ProcessEnv): DevFolderApiResult { - // The exact CLI invocation the product description promises, escaping the CLI's own `/v2` base - // (`design.md`'s Decision #5) - `cwd: REPO_ROOT` matters here since `../actor-runtime/...` resolves - // against the CLI's configured base URL, not the filesystem; it is unrelated to `path` itself, which - // is always this test's own absolute `ACTOR_DIR`. + // The exact CLI invocation `requirements/api.md`'s `/actor-runtime/*` section documents, escaping the + // CLI's own `/v2` base - `cwd: REPO_ROOT` matters here since `../actor-runtime/...` resolves against + // the CLI's configured base URL, not the filesystem; it is unrelated to `path` itself, which is + // always this test's own absolute `ACTOR_DIR`. const output = apify(['api', 'POST', `../actor-runtime/dev-folder/${actorId}`, '--body', JSON.stringify(path)], { cwd: REPO_ROOT, env, @@ -126,8 +126,9 @@ describe('local dev-folder bind mount: edit-compile-call loop with no rebuild (r const actorId = push.actor.id; // Local build, so the host folder already looks like the image's working directory before it - // is ever bind-mounted over it (`design.md`'s "Layout mismatch" risk) - `dist/main.js` for the - // container's own `CMD` to run. The host folder's own `node_modules` (just installed above) is + // is ever bind-mounted over it - `dist/main.js` for the container's own `CMD` to run, + // matching the layout the image itself expects. The host folder's own `node_modules` (just + // installed above) is // deliberately NOT what the container is meant to rely on - the assertion below proves the // anonymous volume, not this directory's own `node_modules`, is what the container actually used. execFileSync('npm', ['run', 'build'], { cwd: ACTOR_DIR, stdio: 'inherit' }); @@ -158,8 +159,8 @@ describe('local dev-folder bind mount: edit-compile-call loop with no rebuild (r expect(log).toContain(EDITED_MARKER); expect(log).not.toContain(`${ORIGINAL_MARKER}\n`); - // An explicit mount line at the top of the run's log (`design.md`'s "Observability of the - // mount"), naming both the host path and the container path being mounted. + // An explicit mount line at the top of the run's log (`actor-driver.md`'s "Observability" + // bullet), naming both the host path and the container path being mounted. expect(log).toContain(ACTOR_DIR); expect(log).toContain(EXPECTED_IMAGE_WORKING_DIR); }, diff --git a/test/integration/dev-folder.test.ts b/test/integration/dev-folder.test.ts index d0ef47a..ef15331 100644 --- a/test/integration/dev-folder.test.ts +++ b/test/integration/dev-folder.test.ts @@ -1,7 +1,8 @@ /** - * Integration coverage for the local dev-folder bind-mount feature's non-Docker-dependent surface - * (`design.md`): the API endpoint's auth/ownership/shape/build-first/probe-classification contract, - * that the registered value never leaks into any `/v2` Actor response, and the console's single-field + * Integration coverage for the local dev-folder bind-mount feature's non-Docker-dependent surface: the + * API endpoint's auth/ownership/shape/build-first/probe-classification contract (`api.md`'s + * `/actor-runtime/*` section), that the registered value never leaks into any `/v2` Actor response, and + * the console's single-field * form (render, submit, clear, redirect, inline error). Every probe outcome is stubbed * (`devFolderDriver` below) - there is no Docker daemon in this sandbox (`docker-driver.ts`'s class doc * comment); the real-probe accept/reject path and the mount itself are only exercised end-to-end in @@ -187,6 +188,24 @@ describe('POST /actor-runtime/dev-folder/:actorId', () => { expect(res.status).toBe(400); }); + it('rejects the same way for an actor whose only successful build is tagged something other than "latest" - no fallback to an arbitrary other tag', async () => { + const driver = devFolderDriver({ ok: true }); + server = await startTestServer(driver); + const actor = await server.client.actors().create({ name: 'non-latest-tag-only-actor' }); + // Mirrors exactly the Actor shape a tag-less `POST /actors/:actorId/runs` would 404 against: a + // successful build exists, but not tagged `latest`. + await seedSucceededBuild((await getRegistries().actors.get(actor.id))!, 'staging'); + + const res = await post(server.baseUrl, actor.id, JSON.stringify('/abs/path'), server.token); + expect(res.status).toBe(400); + expect(res.data.error.type).toBe('dev-folder-not-buildable'); + // The probe must never be reached - there is no image to probe against without a `latest` tag. + expect(driver.probeDevFolderCalls).toEqual([]); + + const stored = await getRegistries().actors.get(actor.id); + expect(stored?.localDevFolder).toBeUndefined(); + }); + it('200s and stores the path when the probe reports ok, for an actor with a successful build', async () => { const driver = devFolderDriver({ ok: true }); server = await startTestServer(driver); diff --git a/test/integration/job-lifecycle.test.ts b/test/integration/job-lifecycle.test.ts index 99a8a06..989f47d 100644 --- a/test/integration/job-lifecycle.test.ts +++ b/test/integration/job-lifecycle.test.ts @@ -314,6 +314,71 @@ describe('job lifecycle: TIMED-OUT mapping and abort/completion race guards', () }); }); + describe('imageWorkingDirectory persists alongside the tagged build in the same write', () => { + it('a successful build outcome carrying imageWorkingDirectory lands it on the Actor together with the tagged build', async () => { + const driver = fixedBuildOutcomeDriver({ imageId: 'x', imageWorkingDirectory: '/usr/src/app' }); + server = await startTestServer(driver); + const actor = await seedActor(server, 'dev-folder-capture-actor'); + + const record: BuildRecord = { + id: generateId(), + userId: actor.userId, + actorId: actor.id, + versionNumber: '0.0', + buildNumber: '0.0.1', + tag: 'latest', + status: 'READY', + startedAt: new Date().toISOString(), + }; + await getRegistries().builds.set(record.id, record); + + await runBuildInBackground(driver, actor, VERSION, record, { tag: 'latest', useCache: true }); + + const final = await getRegistries().builds.get(record.id); + expect(final?.status).toBe('SUCCEEDED'); + + // Both effects of the single `updateActor` call landed together: the tagged build... + const finalActor = await getRegistries().actors.get(actor.id); + expect(finalActor?.taggedBuilds.latest).toEqual({ buildId: record.id, buildNumber: record.buildNumber }); + // ...and the captured working directory, in the same write, not a separate one. + expect(finalActor?.imageWorkingDirectory).toBe('/usr/src/app'); + }); + + it('a successful build outcome with no imageWorkingDirectory leaves a previously stored value untouched', async () => { + const driver = fixedBuildOutcomeDriver({ imageId: 'y' }); + server = await startTestServer(driver); + const actor = await seedActor(server, 'dev-folder-preserve-actor'); + + // A known-good value from an earlier build's inspect, which this build's outcome (no + // `imageWorkingDirectory` at all - e.g. its inspect failed or came up empty/`/`) must not + // clobber with `undefined`. + await updateActor(actor.id, (current) => ({ ...current, imageWorkingDirectory: '/usr/src/app' })); + + const record: BuildRecord = { + id: generateId(), + userId: actor.userId, + actorId: actor.id, + versionNumber: '0.0', + buildNumber: '0.0.2', + tag: 'latest', + status: 'READY', + startedAt: new Date().toISOString(), + }; + await getRegistries().builds.set(record.id, record); + + await runBuildInBackground(driver, actor, VERSION, record, { tag: 'latest', useCache: true }); + + const final = await getRegistries().builds.get(record.id); + expect(final?.status).toBe('SUCCEEDED'); + + // The tag still moves to the new build... + const finalActor = await getRegistries().actors.get(actor.id); + expect(finalActor?.taggedBuilds.latest).toEqual({ buildId: record.id, buildNumber: record.buildNumber }); + // ...but the previously known working directory survives, since this outcome carried none. + expect(finalActor?.imageWorkingDirectory).toBe('/usr/src/app'); + }); + }); + describe('finding 3: run abort is race-proof end to end', () => { it('abort while running: ABORTED sticks despite the completion write racing in after', async () => { const driver = deferredRunDriver(); diff --git a/test/unit/dev-folder-validation.test.ts b/test/unit/dev-folder-validation.test.ts index 9bfe0cd..a68d0ad 100644 --- a/test/unit/dev-folder-validation.test.ts +++ b/test/unit/dev-folder-validation.test.ts @@ -1,6 +1,7 @@ /** - * Pure-function coverage for `validateDevFolderPathShape` (`design.md`: "A cheap shape pre-filter still - * runs first" - absolute POSIX path, no newline/NUL, length cap, `~` never expanded). This is the only + * Pure-function coverage for `validateDevFolderPathShape` (`actor-driver.md`'s "Registration validates + * the path in two layers" bullet: shape pre-filter runs first - absolute POSIX path, no newline/NUL, + * length cap, `~` never expanded). This is the only * layer of validation exercisable with no registries/driver at all; `setDevFolder`'s build-first and * host-side-probe layers are covered by `test/integration/dev-folder.test.ts`. */ diff --git a/test/unit/docker-driver.test.ts b/test/unit/docker-driver.test.ts index 93aeb0d..c47b5e4 100644 --- a/test/unit/docker-driver.test.ts +++ b/test/unit/docker-driver.test.ts @@ -97,7 +97,7 @@ describe('DockerDriver.reconcileOrphans', () => { expect(listContainers).not.toHaveBeenCalled(); }); - it('removes each matched container with { force: true, v: true } (design.md: Volume cleanup - an orphaned devMount run must not leak its anonymous node_modules volume past a restart)', async () => { + it('removes each matched container with { force: true, v: true } (an orphaned devMount run must not leak its anonymous node_modules volume past a restart)', async () => { const { docker, removeCallOptions } = stubDocker([ { Id: 'container-a', Labels: { 'actor-runtime.runId': 'run-a' } }, ]); @@ -256,7 +256,7 @@ describe('DockerDriver.startRun - faithful demuxStream stub (regression: dockero }); }); -describe('DockerDriver.startRun - dev-folder mount composition (design.md: "Applying the mount")', () => { +describe('DockerDriver.startRun - dev-folder mount composition (actor-driver.md: "The mount uses HostConfig.Mounts")', () => { it('adds exactly the bind + anonymous-volume Mounts entries when devMount is present, and never a Binds key', async () => { const stub = stubDockerForRun(); const driver = new DockerDriver(stub.docker); @@ -355,7 +355,7 @@ describe('DockerDriver.startRun - dev-folder mount composition (design.md: "Appl }); }); -describe('DockerDriver container removal passes { v: true } (design.md: "Volume cleanup is in this PR")', () => { +describe('DockerDriver container removal passes { v: true } (actor-driver.md: "container removal passes { v: true }")', () => { it("startRun's finally block removes the container with { v: true }, whether or not the run had a devMount", async () => { const stub = stubDockerForRun(); const driver = new DockerDriver(stub.docker); @@ -374,7 +374,7 @@ describe('DockerDriver container removal passes { v: true } (design.md: "Volume }); }); -describe('DockerDriver.startBuild - imageWorkingDirectory capture (design.md: "Capture of imageWorkingDirectory")', () => { +describe('DockerDriver.startBuild - imageWorkingDirectory capture (actor-driver.md: "imageWorkingDirectory is captured by the driver itself")', () => { /** A stub covering only what `startBuild` calls: `buildImage`, `modem.followProgress` (invoking its * `onFinished` callback synchronously, as a successful build with no progress lines), and `getImage` * for the post-build inspect. */ @@ -457,7 +457,7 @@ describe('DockerDriver.startBuild - imageWorkingDirectory capture (design.md: "C }); }); -describe('DockerDriver.probeDevFolder (design.md: "The check is a create-only probe container, never started")', () => { +describe('DockerDriver.probeDevFolder (actor-driver.md: "a create-only probe container, never started")', () => { it('returns ok and removes the (never-started) probe container on success, without ever calling .start()', async () => { const start = vi.fn(); const remove = vi.fn(async () => undefined); From 86cdbc51ed363108b68336e73fd18f66104ea1f3 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 19 Aug 2026 08:07:12 +0000 Subject: [PATCH 03/11] Distinguish deleted-build 404, share the default build tag A tag whose build record was deleted reported "Actor has no build tagged X", which was wrong: the tag exists. Build resolution now reports the two cases apart, both still 404 record-not-found, and both are covered by tests. The default build tag becomes one exported constant that the run route and the run service import, replacing three hand-synced 'latest' literals. Adds a services-layer test that a run's mount is derived from the Actor's stored fields, covering both set and cleared. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_011L4VcFqN9UZUbVRMvQSugv --- src/api/routes/actors.ts | 20 ++++- src/services/actors.ts | 47 ++++++++---- src/services/runs.ts | 9 +-- test/integration/actors-builds-runs.test.ts | 41 +++++++++- test/integration/dev-folder.test.ts | 83 ++++++++++++++++++++- 5 files changed, 173 insertions(+), 27 deletions(-) diff --git a/src/api/routes/actors.ts b/src/api/routes/actors.ts index 561eabb..c899523 100644 --- a/src/api/routes/actors.ts +++ b/src/api/routes/actors.ts @@ -8,6 +8,7 @@ import { h, jsonBody, paginationParams, queryBoolean, queryNumber, queryString, import { addOrReplaceVersion, createActor, + DEFAULT_BUILD_TAG as DEFAULT_TAG, deleteActor, findVersion, listOwnedActors, @@ -24,8 +25,6 @@ import type { ApiServerDeps } from '../server.js'; import { CONTAINER_API_BASE_URL } from '../../config.js'; import { resolveProxyPassword } from '../../services/users.js'; -const DEFAULT_TAG = 'latest'; - export function mountActors(router: Router, deps: ApiServerDeps): void { router.get( '/actors', @@ -275,8 +274,21 @@ export function mountActors(router: Router, deps: ApiServerDeps): void { if (!actor) throw recordNotFound(); const tag = queryString(req, 'build') ?? DEFAULT_TAG; - const build = await resolveTaggedBuild(actor, tag); - if (!build) throw recordNotFound(`Actor has no build tagged "${tag}"`); + const lookup = await resolveTaggedBuild(actor, tag); + if (!lookup.found) { + // Two distinct 404s, not one generic one: a tag that was never recorded at all vs. a tag + // that *is* recorded but whose `BuildRecord` was since deleted (`DELETE + // /actor-builds/:buildId` does not clear any tag pointing at the deleted build) - see + // `resolveTaggedBuild`'s doc comment. Both keep the same status (404) and error `type` + // (`record-not-found`); only the message differs, so a caller reading the tag genuinely + // still exists is never told it doesn't. + throw recordNotFound( + lookup.reason === 'build-deleted' + ? `Actor's build tagged "${tag}" was deleted` + : `Actor has no build tagged "${tag}"`, + ); + } + const build = lookup.build; const body = rawBody(req); const input = diff --git a/src/services/actors.ts b/src/services/actors.ts index 42a2027..f941f38 100644 --- a/src/services/actors.ts +++ b/src/services/actors.ts @@ -3,23 +3,35 @@ import type { ActorRecord, ActorVersionRecord, BuildRecord } from '../storage/en import { getRegistries } from '../storage/registries.js'; import type { Driver, DevFolderProbeOutcome } from '../driver/types.js'; -/** The tag a build/run resolves to when the caller names none - mirrored by `POST - * /actors/:actorId/runs` (`api/routes/actors.ts`'s own `DEFAULT_TAG`) and by `resolveDevFolderProbeBuild` - * below, which shares this constant so the two can never drift apart. */ +/** The tag a build/run resolves to when the caller names none. `api/routes/actors.ts` imports this + * constant directly (aliased to its own local `DEFAULT_TAG` name) instead of declaring a second + * `'latest'` literal, and `services/runs.ts` does the same for its `DEFAULT_BUILD_TAG` - one exported + * value, three import sites, so the three can no longer drift apart the way three independent literals + * could. */ export const DEFAULT_BUILD_TAG = 'latest'; /** - * Resolves the `BuildRecord` tagged `tag` on this Actor - the exact lookup `POST - * /actors/:actorId/runs` performs (`actor.taggedBuilds[tag]`, then load that build by id), extracted - * here so the registration probe below can resolve an image the identical way a real run does, instead - * of re-implementing it. A missing tag and a missing build record are both reported the same way a - * missing tag is at that route: `null`, with no fallback to any other tag. + * Resolves the tag `tag` on this Actor to its `BuildRecord`, or a reason it couldn't be resolved - the + * exact lookup `POST /actors/:actorId/runs` performs (`actor.taggedBuilds[tag]`, then load that build + * by id), extracted here so the registration probe below can resolve an image the identical way a real + * run does, instead of re-implementing it. The two failure shapes are kept distinguishable rather than + * both collapsing to one generic "not found": `no-such-tag` when `tag` has never been recorded on this + * Actor at all, and `build-deleted` when the tag *is* recorded but the `BuildRecord` it points at is + * gone (builds are deletable via `DELETE /actor-builds/:buildId` - `services/builds.ts`'s `deleteBuild` + * does not clear any tag that pointed at it). Callers that only care about "did this resolve" can check + * `result.found`; callers that need an accurate error message (like the run-start route) can tell the + * two failure shapes apart instead of reporting "no build tagged" for a tag that does, in fact, exist. + * No fallback to any other tag in either failure case. */ -export async function resolveTaggedBuild(actor: ActorRecord, tag: string): Promise { +export type TaggedBuildLookup = + { found: false; reason: 'no-such-tag' | 'build-deleted' } | { found: true; build: BuildRecord }; + +export async function resolveTaggedBuild(actor: ActorRecord, tag: string): Promise { const tagged = actor.taggedBuilds[tag]; - if (!tagged) return null; + if (!tagged) return { found: false, reason: 'no-such-tag' }; const build = await getRegistries().builds.get(tagged.buildId); - return build ?? null; + if (!build) return { found: false, reason: 'build-deleted' }; + return { found: true, build }; } export interface CreateActorInput { @@ -135,6 +147,10 @@ export function recordTaggedBuild(actor: ActorRecord, tag: string, buildId: stri // callers pass an already-unwrapped, already-trimmed string: the API unwraps its JSON-string body, the // console reads its urlencoded form field. +/** Upper bound `validateDevFolderPathShape` rejects a path past - generous enough that no genuine host + * path would ever hit it, just a guard against pathological input. */ +const MAX_DEV_FOLDER_PATH_LENGTH = 4096; + /** Cheap shape pre-filter, run before the host-side existence check, never instead of it (see * `actor-driver.md`'s "Registration validates the path in two layers" bullet). `~` is not expanded - this codebase never * shells out (see `docker-driver.ts`'s class doc comment), so there is no shell to expand it, and @@ -142,8 +158,6 @@ export function recordTaggedBuild(actor: ActorRecord, tag: string, buildId: stri * should assume. Returns `null` for a shape-valid non-empty path, or a human-readable rejection reason. * Exported for direct unit testing as a pure function; the empty-string "clear" case is handled by * `setDevFolder` before this is ever called, not inside it. */ -const MAX_DEV_FOLDER_PATH_LENGTH = 4096; - export function validateDevFolderPathShape(path: string): string | null { if (path.length > MAX_DEV_FOLDER_PATH_LENGTH) { return `Path is too long (max ${MAX_DEV_FOLDER_PATH_LENGTH} characters)`; @@ -170,8 +184,11 @@ export function validateDevFolderPathShape(path: string): string | null { * `latest`-tagged successful build - the build-first precondition. */ async function resolveProbeImageId(actor: ActorRecord): Promise { - const build = await resolveTaggedBuild(actor, DEFAULT_BUILD_TAG); - return build?.imageId ?? null; + const lookup = await resolveTaggedBuild(actor, DEFAULT_BUILD_TAG); + // Both `resolveTaggedBuild` failure reasons ("no such tag" and "tag exists but its build was + // deleted") collapse to the same `null` here - either way there is no build to probe against, so the + // caller reports the same "no successful build" precondition failure for both. + return lookup.found ? (lookup.build.imageId ?? null) : null; } /** Every way `setDevFolder` can end, `ok` included - a discriminated union so both the API route and the diff --git a/src/services/runs.ts b/src/services/runs.ts index 9c90f09..475d871 100644 --- a/src/services/runs.ts +++ b/src/services/runs.ts @@ -6,11 +6,10 @@ import { openKeyValueStore } from '../storage/open.js'; import type { Driver } from '../driver/types.js'; import { appendLog, flushLog, markLogTerminal } from './logs.js'; import { isTerminalJobStatus, transitionJobStatus } from './job-status.js'; -import { findVersion } from './actors.js'; +import { DEFAULT_BUILD_TAG, findVersion } from './actors.js'; const DEFAULT_MEMORY_MBYTES = 1024; const DEFAULT_TIMEOUT_SECS = 300; -const DEFAULT_BUILD_TAG = 'latest'; /** No separate platform disk-default constant exists to match exactly (`apify-core` has no * `ACTOR_DEFAULT_DISK_MBYTES`-shaped constant alongside `ACTOR_DEFAULT_MEMORY_MBYTES`); this mirrors the * 2x ratio `apify-core`'s own OpenAPI examples use for the pair (`packages/consts/src/actors.ts`'s run @@ -50,9 +49,9 @@ export interface StartRunOptions { memoryMbytes?: number; timeoutSecs?: number; /** Build tag or build number this run should use (the real platform's `options.build`) - defaults to - * `'latest'` when omitted, matching `DEFAULT_TAG` in `api/routes/actors.ts` (the route always resolves - * and passes the actual tag it used; this default only matters for direct service-layer callers, e.g. - * tests). */ + * `DEFAULT_BUILD_TAG` (`'latest'`, `services/actors.ts`) when omitted; `api/routes/actors.ts`'s route + * imports that same constant as its local `DEFAULT_TAG` and always resolves and passes the actual tag + * it used, so this default only matters for direct service-layer callers, e.g. tests. */ build?: string; proxyPassword?: string; apiBaseUrl: string; diff --git a/test/integration/actors-builds-runs.test.ts b/test/integration/actors-builds-runs.test.ts index 3a02417..5456785 100644 --- a/test/integration/actors-builds-runs.test.ts +++ b/test/integration/actors-builds-runs.test.ts @@ -153,9 +153,46 @@ describe('actors / versions / builds / runs (via real apify-client)', () => { expect(run.generalAccess).toBe('FOLLOW_USER_SETTING'); }); - it('starting a run against an Actor with no tagged build 404s', async () => { + it('starting a run against an Actor with no tagged build 404s, naming the tag that has no build', async () => { const actor = await server.client.actors().create({ name: 'no-build-actor' }); - await expect(server.client.actor(actor.id).start({})).rejects.toMatchObject({ statusCode: 404 }); + await expect(server.client.actor(actor.id).start({})).rejects.toMatchObject({ + statusCode: 404, + type: 'record-not-found', + message: 'Actor has no build tagged "latest"', + }); + }); + + it('starting a run against a tag whose BuildRecord was since deleted 404s with a message that does not claim the tag is missing (it is not)', async () => { + const actor = await server.client.actors().create({ name: 'deleted-tagged-build-actor' }); + + // Seed a tagged, successful build directly, then delete the underlying BuildRecord without + // clearing the tag - the exact state `deleteBuild` (`services/builds.ts`) leaves behind, since it + // only removes the build itself and never touches `actor.taggedBuilds`. + const { builds } = getRegistries(); + const fakeBuildId = 'fakeBuildId12345d'; + await builds.set(fakeBuildId, { + id: fakeBuildId, + userId: actor.userId, + actorId: actor.id, + versionNumber: '0.0', + buildNumber: '0.0.1', + tag: 'latest', + status: 'SUCCEEDED', + startedAt: new Date().toISOString(), + finishedAt: new Date().toISOString(), + imageId: 'fake-image:latest', + }); + await updateActor(actor.id, (current) => recordTaggedBuild(current, 'latest', fakeBuildId, '0.0.1')); + await builds.delete(fakeBuildId); + + // Before the fix this collapsed to the exact same "Actor has no build tagged" message as the + // no-such-tag case above - factually wrong here, since the tag `latest` genuinely still exists on + // this Actor. Status (404) and type (`record-not-found`) are unchanged either way. + await expect(server.client.actor(actor.id).start({})).rejects.toMatchObject({ + statusCode: 404, + type: 'record-not-found', + message: 'Actor\'s build tagged "latest" was deleted', + }); }); it('a run against a (test-seeded) successful build wires storages/env, then fails fast without Docker', async () => { diff --git a/test/integration/dev-folder.test.ts b/test/integration/dev-folder.test.ts index ef15331..6cc2013 100644 --- a/test/integration/dev-folder.test.ts +++ b/test/integration/dev-folder.test.ts @@ -19,7 +19,7 @@ import { getRegistries } from '../../src/storage/registries.js'; import { generateId } from '../../src/storage/ids.js'; import { recordTaggedBuild, updateActor } from '../../src/services/actors.js'; import type { ActorRecord, BuildRecord } from '../../src/storage/entities.js'; -import type { Driver, DevFolderProbeOutcome } from '../../src/driver/types.js'; +import type { Driver, DevFolderMount, DevFolderProbeOutcome } from '../../src/driver/types.js'; /** * A `Driver` whose only interesting behaviour is `probeDevFolder`, returning a caller-controlled @@ -514,3 +514,84 @@ describe('console: dev-folder registration form on the Actor detail view', () => expect(res.status).toBe(404); }); }); + +/** + * A driver that is "available" and records the `devMount` it was asked to start a container with - + * mirrors `run-env-vars.test.ts`'s `envCapturingDriver`, capturing `ctx.devMount` instead of `ctx.env`, + * so `services/runs.ts`'s actor-fields -> `RunContext.devMount` derivation can be exercised end to end + * through the real `startRun` service path, without a real Docker socket. + */ +function devMountCapturingDriver(): { driver: Driver; getCapturedDevMount: () => DevFolderMount | undefined } { + let capturedDevMount: DevFolderMount | undefined; + const driver: Driver = { + available: true, + async init() {}, + async startBuild() { + throw new Error('not used by this stub'); + }, + async abortBuild() {}, + async startRun(ctx, onLog) { + capturedDevMount = ctx.devMount; + onLog('done\n'); + return { exitCode: 0 }; + }, + async abortRun() {}, + async reconcileOrphans() {}, + }; + return { driver, getCapturedDevMount: () => capturedDevMount }; +} + +describe('run-start devMount derivation (actor fields -> RunContext.devMount, services/runs.ts)', () => { + let server: TestServerHandle; + + afterEach(async () => { + await server.close(); + }); + + it('an Actor with both localDevFolder and imageWorkingDirectory set gets exactly that pair as devMount on the real run-start service path', async () => { + const capturing = devMountCapturingDriver(); + server = await startTestServer(capturing.driver); + const actor = await server.client.actors().create({ name: 'devmount-present-actor' }); + await seedSucceededBuild((await getRegistries().actors.get(actor.id))!); + await updateActor(actor.id, (current) => ({ + ...current, + localDevFolder: '/abs/dev/src', + imageWorkingDirectory: '/usr/src/app', + })); + + const run = await server.client.actor(actor.id).start({}, { waitForFinish: 5 }); + expect(run.status).toBe('SUCCEEDED'); + expect(capturing.getCapturedDevMount()).toEqual({ + localDevFolder: '/abs/dev/src', + imageWorkingDirectory: '/usr/src/app', + }); + }); + + it('an Actor that was never registered gets devMount: undefined on the real run-start service path', async () => { + const capturing = devMountCapturingDriver(); + server = await startTestServer(capturing.driver); + const actor = await server.client.actors().create({ name: 'devmount-never-registered-actor' }); + await seedSucceededBuild((await getRegistries().actors.get(actor.id))!); + + const run = await server.client.actor(actor.id).start({}, { waitForFinish: 5 }); + expect(run.status).toBe('SUCCEEDED'); + expect(capturing.getCapturedDevMount()).toBeUndefined(); + }); + + it('an Actor whose registration was set and then cleared also gets devMount: undefined, not the stale pair', async () => { + const capturing = devMountCapturingDriver(); + server = await startTestServer(capturing.driver); + const actor = await server.client.actors().create({ name: 'devmount-cleared-actor' }); + await seedSucceededBuild((await getRegistries().actors.get(actor.id))!); + await updateActor(actor.id, (current) => ({ + ...current, + localDevFolder: '/abs/dev/src', + imageWorkingDirectory: '/usr/src/app', + })); + await updateActor(actor.id, (current) => ({ ...current, localDevFolder: undefined })); + + const run = await server.client.actor(actor.id).start({}, { waitForFinish: 5 }); + expect(run.status).toBe('SUCCEEDED'); + expect(capturing.getCapturedDevMount()).toBeUndefined(); + }); +}); From d8d2fa475e89ee283df482de451367c8ad6e0279 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 19 Aug 2026 08:52:21 +0000 Subject: [PATCH 04/11] Address final review: extract dev-folder service, stop leaking modifiedAt Registering a dev folder bumped the Actor's modifiedAt, which is exposed on /v2 -- the emulated API could observe a purely local registration. Dev-folder writes no longer touch it, and clearing an unregistered Actor writes nothing. The dev-folder pipeline moves out of the actors service into its own module, which lets it reuse the builds service instead of duplicating a registry read to avoid a cycle. HTTP statuses and error types move to the route layer. A tag whose build was deleted reports the same bare not-found the run route gave before this branch, so run-start behaviour is unchanged throughout. Probe containers are labelled and swept by orphan reconciliation, the driver's probe method is required rather than optional, whitespace-only paths are rejected instead of silently clearing, console styles move into the shared template, and the e2e test copies the sample Actor instead of editing it. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_011L4VcFqN9UZUbVRMvQSugv --- CLAUDE.MD | 6 + requirements/actor-driver.md | 10 +- requirements/api.md | 14 +- requirements/console.md | 3 +- requirements/storage.md | 14 +- src/api/dto/actors.ts | 4 +- src/api/routes/actors.ts | 29 +-- src/api/routes/dev-folder.ts | 70 ++++--- src/api/server.ts | 6 +- src/console/server.ts | 44 ++-- src/console/templates.ts | 18 ++ src/driver/docker-driver.ts | 157 ++++++--------- src/driver/types.ts | 57 ++---- src/services/actors.ts | 201 +------------------ src/services/builds.ts | 33 ++- src/services/dev-folder.ts | 142 +++++++++++++ src/storage/entities.ts | 32 +-- test/e2e/dev-folder-bind-mount.test.ts | 102 +++++----- test/integration/actors-builds-runs.test.ts | 12 +- test/integration/cli-log-stream-race.test.ts | 9 + test/integration/dev-folder.test.ts | 109 +++++++++- test/integration/helpers/test-server.ts | 15 ++ test/integration/identity-resolution.test.ts | 3 + test/integration/job-lifecycle.test.ts | 6 + test/integration/run-env-vars.test.ts | 3 + test/unit/dev-folder-validation.test.ts | 2 +- test/unit/docker-driver.test.ts | 46 ++++- 27 files changed, 634 insertions(+), 513 deletions(-) create mode 100644 src/services/dev-folder.ts diff --git a/CLAUDE.MD b/CLAUDE.MD index 70fe125..1f450e4 100644 --- a/CLAUDE.MD +++ b/CLAUDE.MD @@ -20,6 +20,12 @@ Local Actor runtime is an Actor development tool for developing, running, and de - Use `apify cli api ...` to send API calls and inspect the Actors, builds, runs, storages and other objects. - To simulate multiple users use custom token and in additional authorization header. For example: `apify cli api v2/datasets -H '{"authorization": "Bearer TOKEN"}'` - You can use already authenticated CLI or call `apify login --token TOKEN` +- To iterate on an Actor's source without a rebuild for every change: push and build the Actor once, then + register its local source folder with `apify api POST ../actor-runtime/dev-folder/ --body + '"/abs/path/to/src"'` (the `../` escapes the CLI's own `/v2` base and lands on this runtime's own + `/actor-runtime/*` endpoint). From then on, edit locally, recompile locally (`tsc` or the + language-appropriate equivalent), and `apify call` again - no `apify push`/build in between. Submitting + `--body '""'` clears the registration. Dependency changes still need a real rebuild. ## Through direct API calls - You can also send direct API calls. For example: http://localhost:3333/v2/datasets?token=TOKEN diff --git a/requirements/actor-driver.md b/requirements/actor-driver.md index db1fe5c..6d626a2 100644 --- a/requirements/actor-driver.md +++ b/requirements/actor-driver.md @@ -74,7 +74,10 @@ up either way. - Submitting the **empty string clears the registration** and never runs either validation layer - there is no path to check, and clearing must always succeed, including when Docker itself is - unreachable. + unreachable. Clearing an Actor that has nothing registered is a no-op: no registry write at all. + A **whitespace-only** string (e.g. `" "`) is not a clear - only the literal empty string is; + whitespace-only is trimmed and then rejected by the shape check (400), the same as any other value + that fails to start with `/`. - Errors are classified by shape, most specific first, and every non-success branch rejects rather than guessing: no HTTP response at all (Docker itself unreachable) is reported as "could not verify - Docker is unreachable", never as "does not exist"; the probe's own image returning 404 is an @@ -128,6 +131,11 @@ an empty auto-created directory. - Neither `localDevFolder` nor `imageWorkingDirectory` is ever exposed on the public `/v2` API (`storage.md`). +- **Registering or clearing a dev folder never bumps the Actor's `modifiedAt`.** Unlike + `localDevFolder`/`imageWorkingDirectory` themselves, `modifiedAt` _is_ exposed on `/v2` - bumping it on + every dev-folder write would leak this local-only feature into the emulated API through a timing side + channel, contradicting the whole point of keeping it outside `/v2`. The write goes straight to the + `__ACTORS__` registry rather than through the normal `modifiedAt`-bumping Actor-update path. # Networking diff --git a/requirements/api.md b/requirements/api.md index 8d2900a..f9d6b63 100644 --- a/requirements/api.md +++ b/requirements/api.md @@ -155,12 +155,14 @@ lookup `/v2` uses: a caller can only register a dev folder for their own Actor, and a mismatched or nonexistent `:actorId` answers `404` with error type `record-not-found`, exactly like the rest of the API. - - **Request body**: a JSON string - `'"/abs/path/to/src"'` to set, `'""'` to clear, trimmed after - parsing. This is deliberate, not merely convenient: `apify api`'s `--body` flag validates with - `JSON.parse` and refuses anything that is not valid JSON, so a bare, unquoted path can never reach - this route through the documented CLI invocation at all. A body that is not valid JSON, or is - valid JSON but not a string (a bare number, an object, ...), is rejected with `400` / - `invalid-request` - never silently coerced or stored verbatim. + - **Request body**: a JSON string - `'"/abs/path/to/src"'` to set, `'""'` to clear. Only the literal + empty string clears; a non-empty string is trimmed and then shape-checked, so a whitespace-only + body (e.g. `'" "'`) is rejected as a malformed path, not treated as a clear. This is deliberate, + not merely convenient: `apify api`'s `--body` flag validates with `JSON.parse` and refuses anything + that is not valid JSON, so a bare, unquoted path can never reach this route through the documented + CLI invocation at all. A body that is not valid JSON, or is valid JSON but not a string (a bare + number, an object, ...), is rejected with `400` / `invalid-request` - never silently coerced or + stored verbatim. - **Response**: on success, `{ data: { localDevFolder, imageWorkingDirectory, mountWillApply } }` - the same three values the console detail page shows (`console.md`), doubling as the read-back this design has no separate `GET` for. diff --git a/requirements/console.md b/requirements/console.md index 1cb6731..723a563 100644 --- a/requirements/console.md +++ b/requirements/console.md @@ -54,7 +54,8 @@ JSON) - the console's own port, not the API's - resolving the Actor the same cross-user way every other console read does. It funnels into the same validate-and-persist service function the API endpoint uses, so the two surfaces can never observe or produce different outcomes for the same input. - Submitting an empty value clears the registration, exactly like the API's empty-JSON-string body. + Submitting an empty value clears the registration, exactly like the API's empty-JSON-string body; a + whitespace-only value is rejected as a malformed path instead, also matching the API. - A submission that fails validation (a relative path, an Actor with no successful build, a path the host-side probe could not confirm exists, Docker being unreachable, ...) redirects back to the same detail page with the classified error message shown inline - the build-first rejection and the diff --git a/requirements/storage.md b/requirements/storage.md index a68b1c6..7741982 100644 --- a/requirements/storage.md +++ b/requirements/storage.md @@ -74,12 +74,14 @@ - `localDevFolder` - **optional**. Absent means no dev folder has ever been registered for this Actor. Set/cleared only through `POST /actor-runtime/dev-folder/:actorId` or the console's equivalent form (`api.md`, `console.md`) - never as a side effect of any other Actor write - (pushing a version, starting a build, etc.). Submitting the empty string is a distinct, - first-class "clear" operation, not a stored empty-string value: it removes the field entirely, - the same as if it had never been registered - there is no state that distinguishes "cleared" - from "never set". When present, it is always a non-empty absolute host path that passed both - the shape check and the host-side existence probe at the time it was registered - (`actor-driver.md`). + (pushing a version, starting a build, etc.), and never bumping `modifiedAt` either (see + `actor-driver.md`'s "Registering or clearing a dev folder never bumps `modifiedAt`" bullet - + `modifiedAt` is exposed on `/v2`, unlike this field). Submitting the literal empty string is a + distinct, first-class "clear" operation, not a stored empty-string value: it removes the field + entirely, the same as if it had never been registered, and is a no-op write when there was + nothing to clear. A whitespace-only string does not clear - it is rejected as a malformed path + (`actor-driver.md`). When present, it is always a non-empty absolute host path that passed both + the shape check and the host-side existence probe at the time it was registered. - `imageWorkingDirectory` - **optional**. Absent until at least one build has succeeded and its image's working directory could be captured (`.Config.WorkingDir` via `dockerode`, never by shelling out to a `docker` command-line invocation); also absent when the captured value was diff --git a/src/api/dto/actors.ts b/src/api/dto/actors.ts index 42a9f68..3d1c580 100644 --- a/src/api/dto/actors.ts +++ b/src/api/dto/actors.ts @@ -1,7 +1,7 @@ import type { ActorRecord, BuildRecord, RunRecord } from '../../storage/entities.js'; -/** Matches `services/runs.ts`'s `DEFAULT_BUILD_TAG` - backfilled here only for run records that predate - * `options.build` (directly-seeded test fixtures); every real run always has it set already. */ +/** Matches `services/actors.ts`'s `DEFAULT_BUILD_TAG` - backfilled here only for run records that + * predate `options.build` (directly-seeded test fixtures); every real run always has it set already. */ const DEFAULT_RUN_BUILD_TAG = 'latest'; /** Matches `services/runs.ts`'s `DISK_MBYTES_PER_MEMORY_MBYTE` - backfilled here only for run records * that predate `options.diskMbytes`; every real run always has it set already. */ diff --git a/src/api/routes/actors.ts b/src/api/routes/actors.ts index c899523..5084839 100644 --- a/src/api/routes/actors.ts +++ b/src/api/routes/actors.ts @@ -13,10 +13,15 @@ import { findVersion, listOwnedActors, resolveOwnedActor, - resolveTaggedBuild, updateActor, } from '../../services/actors.js'; -import { listOwnedBuilds, startBuild, waitForBuildFinish, type StartBuildOptions } from '../../services/builds.js'; +import { + listOwnedBuilds, + resolveTaggedBuild, + startBuild, + waitForBuildFinish, + type StartBuildOptions, +} from '../../services/builds.js'; import { listOwnedRuns, startRun, waitForRunFinish } from '../../services/runs.js'; import { getRegistries } from '../../storage/registries.js'; import { actorDto, buildDto, runDto } from '../dto/actors.js'; @@ -276,17 +281,15 @@ export function mountActors(router: Router, deps: ApiServerDeps): void { const tag = queryString(req, 'build') ?? DEFAULT_TAG; const lookup = await resolveTaggedBuild(actor, tag); if (!lookup.found) { - // Two distinct 404s, not one generic one: a tag that was never recorded at all vs. a tag - // that *is* recorded but whose `BuildRecord` was since deleted (`DELETE - // /actor-builds/:buildId` does not clear any tag pointing at the deleted build) - see - // `resolveTaggedBuild`'s doc comment. Both keep the same status (404) and error `type` - // (`record-not-found`); only the message differs, so a caller reading the tag genuinely - // still exists is never told it doesn't. - throw recordNotFound( - lookup.reason === 'build-deleted' - ? `Actor's build tagged "${tag}" was deleted` - : `Actor has no build tagged "${tag}"`, - ); + // `no-such-tag` names the tag, matching base behavior exactly. `build-deleted` (the tag + // exists, but its BuildRecord was removed via `DELETE /actor-builds/:buildId`, which does + // not clear the tag pointing at it) throws the same bare `recordNotFound()` base did for + // this case too - not a custom message - so run-start stays byte-for-byte base-identical + // for every input class; `resolveTaggedBuild` (services/builds.ts) only exists so this + // route and the dev-folder probe can each still branch on *which* reason it was, without + // duplicating the tag/build lookup itself. + if (lookup.reason === 'build-deleted') throw recordNotFound(); + throw recordNotFound(`Actor has no build tagged "${tag}"`); } const build = lookup.build; diff --git a/src/api/routes/dev-folder.ts b/src/api/routes/dev-folder.ts index 74db10e..cf80c66 100644 --- a/src/api/routes/dev-folder.ts +++ b/src/api/routes/dev-folder.ts @@ -1,30 +1,55 @@ /** * `POST /actor-runtime/dev-folder/:actorId` - deliberately outside the emulated `/v2` surface - * (`api.md`'s `/actor-runtime/*` namespace), so this is mounted directly on the API `app`, not the `v2` - * router (`server.ts`'s "Auth is per-router, not global" note) - it therefore needs its own `auth()`, - * applied to a small router of its own below, not inherited from `v2`. + * (`api.md`'s `/actor-runtime/*` namespace), so this router is mounted directly on the API `app` + * (`server.ts`), not nested under the `v2` router - it needs its own `auth()` rather than inheriting + * `v2`'s. * - * Canonical body is a JSON string: `'"/abs/path"'` to set, `'""'` to clear (`api.md`'s `/actor-runtime/*` - * section - `apify api`'s own `--body` validates with `JSON.parse` and refuses anything that isn't - * valid JSON, so a bare, unquoted path can never reach this route through the documented CLI invocation - * at all). A JSON value that parses but isn't a string (a number, an object, ...) is rejected the same - * way a malformed body is - only a genuine JSON string is ever a valid registration payload. + * Canonical body is a JSON string: `'"/abs/path"'` to set, `'""'` to clear (`api.md`). A JSON value that + * parses but isn't a string is rejected the same way a malformed body is. * - * Ownership-scoped like every other Actor write on this API port: `resolveOwnedActor` (not the - * console's cross-user `getActorById`) so a caller can only ever register a dev folder for their own - * Actor, and can name it by id, plain name, or `username~name` the same way `POST .../builds` and - * `POST .../runs` already do. + * Ownership-scoped like every other Actor write on this API port: `resolveOwnedActor`, so a caller can + * only ever register a dev folder for their own Actor. */ -import express, { type Express } from 'express'; +import express, { type Router } from 'express'; import { auth, requireUser } from '../auth.js'; import { sendData } from '../envelope.js'; -import { ApiError, recordNotFound } from '../errors.js'; +import { ApiError, invalidRequest, recordNotFound } from '../errors.js'; import { h, jsonBody } from '../handler.js'; -import { describeDevFolderError, devFolderStatus, resolveOwnedActor, setDevFolder } from '../../services/actors.js'; +import { + describeDevFolderFailure, + devFolderStatus, + setDevFolder, + type SetDevFolderResult, +} from '../../services/dev-folder.js'; +import { resolveOwnedActor } from '../../services/actors.js'; import type { ApiServerDeps } from '../server.js'; -export function mountDevFolder(app: Express, deps: ApiServerDeps): void { +/** Maps a non-`ok` `SetDevFolderResult` to the `ApiError` this route throws - the HTTP status and API + * error `type` are this route's own concern, not the service layer's (`describeDevFolderFailure` only + * supplies the message text, shared with the console). */ +function toApiError(result: Exclude): ApiError { + const message = describeDevFolderFailure(result); + switch (result.kind) { + case 'invalid-path': + return invalidRequest(message); + case 'no-successful-build': + return new ApiError(400, 'dev-folder-not-buildable', message); + case 'not-found': + return new ApiError(400, 'dev-folder-path-not-found', message); + case 'unreachable': + return new ApiError(503, 'dev-folder-check-unavailable', message); + case 'image-missing': + return new ApiError(500, 'internal-error', message); + case 'unknown': + return new ApiError(400, 'dev-folder-check-failed', message); + } +} + +/** Builds the `/actor-runtime/dev-folder` router - `server.ts` mounts it itself + * (`app.use('/actor-runtime', devFolderRouter(deps))`), matching every other `mount*` route module's + * convention of owning its route pattern, not the mount path. */ +export function devFolderRouter(deps: ApiServerDeps): Router { const router = express.Router(); router.use(auth()); @@ -37,18 +62,13 @@ export function mountDevFolder(app: Express, deps: ApiServerDeps): void { const raw = jsonBody(req); if (typeof raw !== 'string') { - throw new ApiError( - 400, - 'invalid-request', + throw invalidRequest( 'Request body must be a JSON string - e.g. "/abs/path/to/src" to set, or "" to clear', ); } - const result = await setDevFolder(deps.driver, actor, raw.trim()); - if (result.kind !== 'ok') { - const info = describeDevFolderError(result); - throw new ApiError(info.status, info.type, info.message); - } + const result = await setDevFolder(deps.driver, actor, raw); + if (result.kind !== 'ok') throw toApiError(result); // The response body doubles as the read-back - there is deliberately no separate `GET` for // this yet - with the same three fields the console detail page shows. @@ -56,5 +76,5 @@ export function mountDevFolder(app: Express, deps: ApiServerDeps): void { }), ); - app.use('/actor-runtime', router); + return router; } diff --git a/src/api/server.ts b/src/api/server.ts index 5c51af2..8fcff5a 100644 --- a/src/api/server.ts +++ b/src/api/server.ts @@ -13,7 +13,7 @@ import { mountBuilds } from './routes/builds.js'; import { mountRuns } from './routes/runs.js'; import { mountLogs } from './routes/logs.js'; import { mountRunStorageAliases } from './routes/run-storage-aliases.js'; -import { mountDevFolder } from './routes/dev-folder.js'; +import { devFolderRouter } from './routes/dev-folder.js'; import type { Driver } from '../driver/types.js'; export interface ApiServerDeps { @@ -45,8 +45,8 @@ export function createApiServer(deps: ApiServerDeps): Express { // `/actor-runtime/*` - a deliberately non-Apify, local-runtime-only namespace (`api.md`), registered // before the 501/404 catch-all below but outside the `v2` router entirely, so it needs its own - // `auth()` (see `mountDevFolder`'s doc comment) rather than inheriting `v2.use(auth())` above. - mountDevFolder(app, deps); + // `auth()` (see `devFolderRouter`'s doc comment) rather than inheriting `v2.use(auth())` above. + app.use('/actor-runtime', devFolderRouter(deps)); app.use((req: Request, res: Response) => { const path = req.path.replace(/^\/+/, ''); diff --git a/src/console/server.ts b/src/console/server.ts index 043f60a..3b1f006 100644 --- a/src/console/server.ts +++ b/src/console/server.ts @@ -16,13 +16,8 @@ */ import express, { type Express } from 'express'; -import { - describeDevFolderError, - devFolderStatus, - getActorById, - listAllActors, - setDevFolder, -} from '../services/actors.js'; +import { getActorById, listAllActors } from '../services/actors.js'; +import { describeDevFolderFailure, devFolderStatus, setDevFolder } from '../services/dev-folder.js'; import { getBuildById, listAllBuilds } from '../services/builds.js'; import { getRunById, listAllRuns } from '../services/runs.js'; import { getFullLog } from '../services/logs.js'; @@ -32,7 +27,7 @@ import { openDataset, openKeyValueStore, openRequestQueue } from '../storage/ope import { pageKeys } from '../services/kv-key-listing.js'; import { applyDatasetProjection, type DatasetItem } from '../services/dataset-projection.js'; import { ansiToHtml } from './ansi.js'; -import { definitionList, escapeHtml, layout, table, type LinkedCell } from './templates.js'; +import { definitionList, devFolderForm, escapeHtml, layout, table, type LinkedCell } from './templates.js'; import type { Driver } from '../driver/types.js'; /** A run's default-storage id rendered as a link to that storage's detail view instead of plain text. */ @@ -49,9 +44,6 @@ export interface ConsoleServerDeps { * through from the POST handler's redirect query param below, since a redirect itself carries no state * of its own. */ function devFolderSection(actorId: string, status: ReturnType, errorMessage?: string): string { - const errorHtml = errorMessage - ? `

Error: ${escapeHtml(errorMessage)}

` - : ''; return ( '

Local dev folder

' + definitionList([ @@ -59,13 +51,7 @@ function devFolderSection(actorId: string, status: ReturnType` + - ` ' + - '' + - '' + - '

Submit an empty value to clear the registration.

' + devFolderForm(actorId, status.localDevFolder ?? '', errorMessage) ); } @@ -127,15 +113,10 @@ export function createConsoleServer(deps: ConsoleServerDeps): Express { res.send(layout(`Actor ${actor.name}`, body)); }); - /** - * The console's one mutation (`console.md`'s "Local dev-folder registration form" section) - - * funnels through the exact same `setDevFolder` the API's `POST /actor-runtime/dev-folder/:actorId` - * uses, resolving the Actor cross-user by the id already in the page URL (no token, matching the - * console's existing unauthenticated reads) rather than through `resolveOwnedActor`. A failure - * redirects back with the classified message in a query param - `describeDevFolderError`'s wording, - * not a bespoke one - so the build-first rejection and the does-not-exist/could-not-verify - * distinction are surfaced, not swallowed. - */ + /** The console's one mutation - funnels through the same `setDevFolder` the API endpoint uses, + * resolving the Actor cross-user by the id already in the page URL (no token) rather than through + * `resolveOwnedActor`. A failure redirects back with `describeDevFolderFailure`'s message in a query + * param, so it's surfaced inline rather than swallowed by the redirect. */ app.post('/actors/:id/dev-folder', async (req, res) => { const actor = await getActorById(req.params.id); if (!actor) { @@ -143,12 +124,15 @@ export function createConsoleServer(deps: ConsoleServerDeps): Express { return; } const body = req.body as Record | undefined; - const submitted = typeof body?.localDevFolder === 'string' ? body.localDevFolder.trim() : ''; + // Not trimmed here - `setDevFolder` itself distinguishes an explicit clear (the literal empty + // string) from a whitespace-only submission (rejected, not treated as a clear); trimming here + // first would collapse that distinction before it ever reaches the service. + const submitted = typeof body?.localDevFolder === 'string' ? body.localDevFolder : ''; const result = await setDevFolder(deps.driver, actor, submitted); if (result.kind !== 'ok') { - const info = describeDevFolderError(result); - res.redirect(`/actors/${encodeURIComponent(actor.id)}?devFolderError=${encodeURIComponent(info.message)}`); + const message = describeDevFolderFailure(result); + res.redirect(`/actors/${encodeURIComponent(actor.id)}?devFolderError=${encodeURIComponent(message)}`); return; } res.redirect(`/actors/${encodeURIComponent(actor.id)}`); diff --git a/src/console/templates.ts b/src/console/templates.ts index 0dd1c44..6be72dc 100644 --- a/src/console/templates.ts +++ b/src/console/templates.ts @@ -36,6 +36,8 @@ export function layout(title: string, body: string): string { dt { font-weight: 600; } pre { background: #f5f5f5; padding: 1rem; overflow-x: auto; white-space: pre-wrap; } .empty { color: #777; font-style: italic; } + .error { color: #b00020; } + .wide-input { width: 28rem; } h1 { margin-top: 0; } @@ -84,6 +86,22 @@ export function table( return `${head}${body}
`; } +/** The dev-folder registration form on the Actor detail view - a single text field plus a submit + * button, styled via this file's shared `