portrait: make the error space real; unblock the sandbox build

[aaylward]

Closes #1267.

Two things, separable by commit: the portrait error-space work the issue asks for, and the sandbox tooling that made it verifiable.

The issue's finding #1 was wrong, and correcting it changed the fix

I filed #1267 saying an exception escaping the handler might be a crash under smithy-cpp's -fno-exceptions posture, and that the question wanted settling before anything else got designed. Settled (comment):

MoonBase sets -fno-exceptions nowhere, and every smithy-cpp server transport — Beast, socket, and loopback — routes the handler through smithy::http::InvokeHandlerGuarded, which contains a throw as a correlated 500. ADR-0003 governs smithy-cpp's public API, not its consumers' compile flags. So this was a contract bug, not a safety one:

• TracerService::trace returns absl::StatusOr, which says every failure is a value. It wasn't.
• The same class of failure produced two different 500 bodies depending on how it failed — the guard's {"message":"internal error","correlationId":…} for a throw, the generated server's {"__type":"InternalFailure",…} for a returned Error::Unknown.
• The throw is genuinely reachable, which I also hadn't established: pngpp::imageToPng throws PngException from libpng's error handler, and a 1200×1200 Image is ~34 MB before toRGB() copies it.

Because loopback goes through the same guard as Beast, all of this is testable in-process.

A sixth finding the issue missed

The generated ErrorToResponse's modeled branch ends:

return helpers::JsonError(400, error.code(), error.message(), {});   // fall-through

A modeled error whose code string matches no declared shape doesn't fail loudly — it becomes a 400 carrying error.message() verbatim. So a typo in a code string is simultaneously the wrong status and the leak finding #2 is about, from something the compiler cannot catch. That's why every modeled error here gets a typed-detail assertion rather than just a code check: recovering detail() is the only thing that distinguishes "declared" from "misspelled". Pinned as its own test so the trap is visible in the suite.

What changed

TracerService::trace returns instead of throwing — bad_alloc → kResourceExhausted, other std::exception → kInternal, plus a catch (...). what() goes to the log, never the wire. The whole body is guarded, not just the render, and the cache write is best-effort: a failure to store must not discard an image that already rendered. do_trace, lookupCache, and storeInCache are protected virtual seams; SmithyTracerHandler takes the service by unique_ptr so a test can install a failing renderer and drive the real handler.

One new modeled error. RenderCapacityError — @error("server") @httpError(503) @retryable — for the one failure a caller can act on: ask for a smaller output. 503 because the condition is a property of the server's capacity right now, not of the request.

The mapping became a contract. ToSmithyError is a free function pinned by a table over every canonical absl::StatusCode, with EveryStatusCodeHasARow keeping the table complete. It pins the mapping; it cannot notice a new source of a status, and says so.

InvalidSceneError gained an optional field naming the offending member as a JSON-pointer path, in the same /member form ValidationException already uses. Optional rather than @required because the cross-field rules — camera vs focus, aspect ratio — have no single member to blame, and they leave it absent.

Pushing back on half of finding #4

Throttling is already handled one layer out: main.cc composes aura::ProductionChain with a sliding-window limiter (20 req/60s per client) and retry_after, so overload answers 429 before the operation is reached. There is also no deadline anywhere in the render path. Modeling ThrottlingError or a timeout would advertise errors the handler cannot return — the model would be describing something untrue. Only the memory ceiling got modeled, because only it has a real client recovery.

That ordering is now pinned by TheRateLimiterAnswersBeforeTheOperationIsReached, which counts handler invocations across the refused request rather than reading its status. The audit section below explains why the status alone was not enough.

The sandbox tooling

While verifying the above I hit the reason portrait was under-tested in the first place: the proxy 403s GitHub source archives, which is how most BCR modules fetch. That blocked boost (so every Beast target), libpng (so portrait and tracy_demo), opentelemetry-cpp, and — via gazelle's cel-spec dep — bazel run //:buildifier, meaning scripts/format-all could not run and the Bazel formatting CI gate could not be checked locally at all.

scripts/make-git-overrides.sh is smithy-cpp's bazel/make-git-overrides.sh adapted here. It scans MODULE.bazel.lock for modules whose source URL is a blocked archive endpoint, clones each at its pinned tag, and replays the BCR's patches, overlay files, and registry MODULE.bazel from bcr.bazel.build — registry metadata isn't blocked. Three things MoonBase's graph needs that smithy-cpp's doesn't, each commented at the site: commit-pinned modules (clone --branch only takes refs), a strip_prefix pointing into a subdirectory, and retrying fetches. It also covers the repos that arrive via module extensions and so never appear in the lockfile scan: the bats toolchain and raylib.

scripts/bazel_restricted_egress.sh is deleted as superseded. That corrects a documented claim which had been discouraging exactly this fix — it said a BCR-overlay library "cannot be overridden from a bare clone", true as far as it goes, but the overlay is served by bcr.bazel.build, which isn't blocked.

Review panel

Ran, three lenses, findings and disposition here. It found a real defect (the cache-write failure above — none of my nine mutations could have caught it), a regression in my own tooling (an inherited EXTRA_MODULES line that downgraded rules_perl and broke @openssl loading), and twelve surviving mutations, all now killed. Everything actionable is fixed in 620c7ea.

Follow-up audit: three claims that were arguments, not tests

After the panel I re-read the diff against the working agreement's "a test beats an argument" rule, asking which claims here required reasoning and got only prose. The error-space work came out clean — every finding the issue raised, plus the sixth one it missed, has a test named after it. Three claims did not. 149d7cd closes them, and one of the three turned out to be false as written.

1. The throttling pushback above. I cited production_chain_test passing 5/5 as evidence, but that suite predates this PR and asserts only the 429 status. A chain that rendered first and rate-limited second would satisfy every assertion in it while making the decision to omit ThrottlingError wrong. The new test counts handler invocations across the refused request. Confirmed by substituting a middleware that calls next() before refusing: the pre-existing test still passes, the new one fails — which is the gap, demonstrated.

2. The deployment claim, corrected below. The audit is what caught it.

3. -fno-exceptions set nowhere — the fact that makes this a contract bug rather than a safety one, and so the fact that redirected the whole fix. Checked by grep, never pinned. Now a #error on __cpp_exceptions at the point trace() relies on it. The try/catch would fail to compile under the flag regardless; the difference is a message about the contract being broken rather than one about exception syntax. No runtime test: exceptions being enabled is a build property, and a try/catch asserting it would only restate what the compiler already guarantees.

Verification

//domains/graphics/apis/portrait/...	5/5, including the Beast-dependent production_chain_test
//domains/games/apis/golf_hub/...	14/14 (the capturing recorder moved to a shared target)
//domains/platform/libs/aura/..., otel/...	2/2, 1/1
//deploy/consolidated:consolidated_test	1/1, new — the deployment invariants below
Mutation testing	9 pre-panel + 12 post-panel + 9 post-audit, all killed
bazel build --nobuild //...	analyses clean, every fetch resolves, no exclusion list
scripts/format-all, //:buildifier_test, gazelle --mode=diff	all clean
MODULE.bazel.lock	untouched — overrides run under --lockfile_mode=off

The documented .bazelrc.user recipe was executed verbatim from an empty file rather than eyeballed, which is how a hardcoded-/root bug got caught: a bazelrc import expands neither ~ nor $HOME.

Deployment check, since a 503 on /trace is new — corrected, because the first version of this paragraph was wrong. Nothing in the deployment treats a 5xx on the trace route as evidence the container is sick, so a 503 cannot eject the backend. Caddy does no passive health checking, and portrait declares no healthcheck at all — it serves /health, but nothing probes it, neither in compose.yaml nor as a Dockerfile HEALTHCHECK. An earlier revision of this description claimed "portrait's healthcheck is a separate /health"; that was not true. The conclusion survives, the stated reason did not. Both halves are now pinned by //deploy/consolidated:consolidated_test against the directives that would falsify them — Caddy's passive-ejection set, an active probe aimed at an operation route, and a compose healthcheck on the trace route, which would restart the container over a render the client is meant to retry smaller.

Follow-up filed, out of scope: #1271 (LRUCache::get leaves a dangling iterator if push_front throws — pre-existing, reachable only under the same OOM).

[cloudflare-workers-and-pages]

Deploying with    Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status	Name	Latest Commit	Preview URL	Updated (UTC)
✅ Deployment successful! View logs	1d4-web	149d7cd	Commit Preview URL Branch Preview URL	Jul 31 2026, 03:33 PM

[aaylward]

Review panel: 3 lenses, everything actionable fixed in 620c7ea

Correctness / control flow; tests and verification; build, docs, tooling. Each agent refuted its own findings before reporting, and I verified every survivor before acting. Two of the three found something that changes the diff.

The defect (lens A)

cache_.insert sat inside the render's try, one line after the response was fully built. A bad_alloc from the cache write therefore discarded an image that had already rendered and answered 503 "render exceeded available memory; try a smaller output" — telling the client to retry smaller for work that had succeeded, and sending the retry back through the same 34 MB render. It's also the peak-allocation instant of the call (image, png_bytes, the response's copy, and the cache's copy all live at once), so it's the most likely allocation in trace() to fail.

Verified against the source: LRUCache::insert(const key_type&, const value_type&) takes a const ref, so the std::move(png_bytes) I wrote was a no-op and the insert allocated a second full PNG.

The sharpest part of the report is why my nine mutations couldn't have caught it: do_trace was the only seam, and it fires before the response exists. Storing is now best-effort behind a lookupCache/storeInCache seam pair — the fix and the testability are the same change. Same lens also caught the cache-hit path sitting outside the guard entirely, so a bad_alloc copying a cached PNG escaped as the transport's generic 500 with neither failure counter incremented; the guard now covers the whole body, which is what the header already claimed.

A regression in my own tooling (lens C)

EXTRA_MODULES="rules_perl/0.5.0" — copied verbatim from smithy-cpp, comment and all — was wrong on both halves here. Our lockfile does record rules_perl, at 1.1.0, whose BCR source is a /releases/download/ asset the proxy allows, so the scan rightly skipped it. And --override_module forces the version, so the line downgraded the graph under @openssl (reached via curl, boost.asio, postgres), which failed to load with no such attribute 'perlopt' in 'perl_binary' rule and took every bazel query reaching it down with it. Reproduced, removed, verified fixed. The list is deleted rather than corrected, and the comment now explains why adding one back is dangerous — that recipe was in the docs too, and is rewritten.

Also from C: raylib's tag is now read out of bazel/extensions/raylib.bzl rather than hardcoded (--override_repository wins silently, so a bumped raylib would otherwise keep building stale sources with nothing reporting it), and absl::Cord had neither its include nor its :cord dep — compiling only because absl/status/status.h happens to pull cord.h in and layering_check is off repo-wide.

Twelve surviving mutations (lens B)

It ran 15; 12 survived. All twelve are killed now, re-run to confirm:

• Metric labels. out_of_memory is the operator's only signal separating an OOM render from any other, and nothing asserted it. That needed a seam — MetricsRecorder was held by value — so TracerService takes a shared_ptr, and golf_hub's CapturingMetricsRecorder is promoted to //domains/platform/libs/futility/otel:capturing_metrics_recorder rather than copied. Asserted in both directions; a one-sided check passes if both branches record the same label.
• << e.what(). Two tests pinned that the cause never reaches the status or the wire; nothing pinned that it still reaches the operator, so deleting it left the suite green while erasing the only diagnostic for every non-OOM render failure. absl::ScopedMockLog pins the positive twin.
• Five field paths in types.cc, whose test file the original change never touched. B was honest that these are unreachable through the generated server today — the constraint traits intercept them, and it checked whether a JSON overflow literal was a way in (it isn't; the decoder 400s first). Pinned anyway: types is a public library and invalidField is new public API.
• The "one convention" claim. PortraitConstraintTest matched substrings, and every member name also appears in its own message text — so the fieldList "path" key could be renamed, retyped, or dropped and all 13 rows stayed green. Now asserted exactly, plus a test making the cross-layer agreement its own subject: radius 0 (handler) and radius 20000 (server) must name /scene/spheres/0/radius identically.

Two comments were overclaiming and now say what's true: the mapping table cannot notice a new source of a status, only a changed mapping — a first-time kFailedPrecondition already has a green row reading UnknownError; and OutOfMemoryIsARetryable503WithTypedDetail's retryable() assertion pins the model's @retryable trait, which the generated client hardcodes, not the handler's flag. EveryStatusCodeHasARow's bound now comes from absl rather than a hardcoded 16.

Simplifications

validateColor was dead — unused parameter, unconditional OkStatus, two four-line call sites — and the knowledge it carried about Color being unsigned char moved onto the type. validateVec3 takes const Vec3& plus its path, deleting both const_casts and stopping the flattening of "Vec3 contains NaN" into "Invalid sphere center". The payload URL is passed as the string_view the API already takes instead of allocating a std::string per failure.

Declined, with reasons

No Retry-After on the 503: memory pressure has no knowable duration, and a fabricated number is worse than a bare 503. Recorded in the model instead, along with the consequence A flagged — the generated client maps any 503 to RenderCapacityError, so callers must branch on detail<RenderCapacityError>() rather than code() to tell the renderer running out of memory from something in front of it being down.

Panel claims I confirmed rather than acted on

AFailedRenderIsNotCached is not vacuous (B planted the exact bug it names and it was killed, along with two other cache mutations). AThrowingHandlerIsContainedAsACorrelated500 fails cleanly rather than crashing when the guard is bypassed. The TheSameHarnessSucceedsWithAWorkingRenderer control is real.

Verification

portrait 5/5 · golf_hub 14/14 · aura 2/2 · otel 1/1 · bazel build --nobuild //... clean · 12 mutations re-run and killed.

One follow-up worth its own issue, out of scope here: LRUCache::get erases the list node before push_front, so a throw from that push_front leaves a dangling iterator that a later get on the same key would erase. Pre-existing in futility/cache, untouched by this diff, and only reachable under the same OOM.

---

Generated by Claude Code