Skip to content

fix(container-runtime): report RPC failures by ErrorKind, and log them to the service - #3640

Open
helix-nine wants to merge 2 commits into
masterfrom
fix/container-runtime-rpc-error-taxonomy
Open

fix(container-runtime): report RPC failures by ErrorKind, and log them to the service#3640
helix-nine wants to merge 2 commits into
masterfrom
fix/container-runtime-rpc-error-taxonomy

Conversation

@helix-nine

@helix-nine helix-nine commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Alternative to #3613, which @dr-bonez rejected on the grounds that a JSON-RPC message should be stable and enumerable — which is why the text lives in data.details. He is right, and the situation is a bit worse than "wrong fix": on the two paths #3613 patches, the OS cannot read message at all.

What the protocol actually is

From<Error> for RpcError (error.rs:512) sends code: kind as i32 and message: kind.as_str() — an i18n lookup. From<RpcError> for Error (:529) reads code back into an ErrorKind and takes the text from data.details, falling back to message only when data is absent (ErrorData::from(&RpcError), :474-489). start_core::Error has no message field for it to land in. So code is the machine key, message is that key's fixed label, data.details is the text.

RpcListener.ts already answers that way twice — -32601 Method not found and -32602 invalid params, both with data.details. Three other sites don't.

Why #3613 changes nothing

Both paths it patches always populate data.details, so message is discarded before every human-facing sink: Display for Error is "{kind.as_str()}: {details}"; statusInfo.error stores an ErrorData, which has no message field; the UI card renders error?.details; start-cli prints the OS's own message plus details. The text was already arriving. What was lost is the category.

What is actually broken

code. 0 is not an ErrorKind variant (the enum starts at 1) and 1 is Unknown, so every container-runtime failure renders as Unknown Error: <text> even though a real variant for it exists at 59 and is translated into all five locales.

getResult (:450-461) — untouched by #3613, and the one site that genuinely does what @dr-bonez is objecting to: it puts free text in message and sends no data, so the Rust fallback lifts it into details. That is the path every action, getInput and createBackup takes. It also forwarded an Embassy-v1 package-defined error-code straight into code, where the OS reads it as an ErrorKind — a v1 package returning 7 would have been reported as Incorrect Password.

handleRpc's catch never printed anything. This is the operator-visible half, and it is what #3613 was reaching for. A package's log stream is this container's own journal — PersistentContainer::new bind-mounts the log dir to /var/log/journal/<machine_id>, LogSource::Package reads it with journalctl -u container-runtime.service -D … (logs.rs:628), and pipe-wrap relays node's stderr into it, which is why Initializing... shows up in package logs today. So when a package's main rejected, the runtime serialised the exception to the socket and dropped it, service_actor.rs:75 sent it to the host journal via log_err(), and the service's own log went quiet at the exact moment it died while the actor retried every 10s. One console.error puts it in the log the operator is already reading.

This PR

  • One errorKind table; message is selected from it, never computed.
  • handleRpcServiceRuntime (59). mapErrorInvalidRequest (38): a malformed line or a dispatch failure is a different class from "we called into the service's code and it threw", and the OS has a variant for each.
  • Second commit renames ErrorKind::JavascriptServiceRuntime ("Service Runtime Error") per @dr-bonez — the old name dates from the embedded Deno runtime. Discriminant 59 is unchanged, since it is the wire value. Its two existing call sites in pack.rs both run a package's own bundled index.js, so the new name fits them too.
  • getResult moves its text to data.details, gains debug, and stops forwarding the v1 error-code as an ErrorKind. The OS gets the same details string it got before.
  • handleRpc logs the exception with the procedure that raised it, using the idiom already at RpcListener.ts:244.
  • RPCSpec.md documents the error object, which it never did — the enumeration written down is the durable half of this.

Notes

Two things worth a decision rather than my assumption:

  • The v1 error-code passthrough. SystemForEmbassy (index.ts:174,178) throws { error, code }, where code is the package's own number. Feeding it into an ErrorKind slot is the bug above, but if you want it preserved it should go somewhere the OS won't misread — data.info, or appended to details.
  • Log volume. SYNC_RETRY_COOLDOWN_SECONDS is a flat 10s with no cap, so a service that fails to start now writes a stack into its journal every 10s indefinitely. That is the same rate it already floods the host journal at, but it is an argument for taking the backoff / statusInfo.error half of A package whose main rejects restart-loops silently: the error reaches neither the service log nor statusInfo.error #3614 in the same release rather than after it.

The rest of #3614 — routing the transition error into statusInfo.error, bounding the retry, discharging the // TODO: ideally this error should be sent to service logs — is Rust-side and stays open.

Credit to @JesseMarkowitz, who found this and wrote #3613; happy to close this in favour of that branch if he'd rather carry it.

Changelog: entry added under the existing ## [0.4.0.2], which has no origin tag (latest is start-os/v0.4.0.1), so no new heading and no version module.

Verification

  • cargo check -p start-core — clean
  • make container-runtime-test — 2 suites, 12 tests, 6 snapshots, pass
  • npm --prefix projects/start-os/container-runtime run check — clean
  • prettier --check on the touched TS/markdown, rustfmt --check on the touched Rust — clean
  • Throwaway test asserting ErrorKind::ServiceRuntime.as_str() resolves in all five locales and that the discriminant is still 59 — passed, then removed

@dr-bonez

Copy link
Copy Markdown
Member

rebase this. also "Javascript Engine Error" is a legacy error code from when we had an embedded deno runtime. You can replace this error code with a "Service Runtime Error".

@helix-nine
helix-nine force-pushed the fix/container-runtime-rpc-error-taxonomy branch from 031a61b to d3538d3 Compare August 11, 2026 00:06
…m to the service

The runtime sent `code: 0` / `code: 1` with `message: typeof error` — the
string "object" — on its two catch-all error paths, and a third path in
`getResult` put free error text in `message` with no `data` at all. StartOS
reads `code` as a `start-core` ErrorKind and takes its text from
`data.details`, so every runtime failure arrived as `Unknown Error`, and the
`getResult` text survived only through the `message` fallback.

Each path now sends the ErrorKind its failure actually is and the fixed label
for that code, with the text in `data.details`, matching the two sites in this
file that already answered `-32601` / `-32602` that way.

`handleRpc`'s catch also never printed the exception. A package's log stream is
this container's own journal, so a service whose `main` threw went silent in
`Logs` at the moment it failed while the actor retried every ten seconds — the
exception reached only the host journal. It is now logged next to the procedure
that raised it.

RPCSpec.md gains the error object it never documented.
The name dates from the embedded Deno runtime. Both of its call sites run a
package's own bundled `index.js`, and it is now also what a container-runtime
procedure failure reports, so "Service Runtime Error" is what it names.

Discriminant 59 is unchanged — it is the wire value the container runtime and
older clients send.
@helix-nine
helix-nine force-pushed the fix/container-runtime-rpc-error-taxonomy branch from d3538d3 to 27763f4 Compare August 11, 2026 00:06
@helix-nine

Copy link
Copy Markdown
Contributor Author

Rebased onto 909a6f0 (the conflict was two ### Fixed entries at the top of the 0.4.0.2 changelog; both kept).

Renamed the kind rather than adding one — ErrorKind::JavascriptServiceRuntime, error.javascripterror.service-runtime, "Service Runtime Error" in all five locales. Its only two existing call sites (s9pk/v2/pack.rs:668,896) shell out to node to read a package's own bundled index.js, so the new name describes them as well as it does the container-runtime path. Kept discriminant 59 — it's the value on the wire, and From<RpcError> for Error reads it back through try_into, so renumbering would strand any client sending the old one.

Split into its own commit so the rename reads separately from the RPC fix.

Verified: cargo check -p start-core, make container-runtime-test (12 tests, 6 snapshots), tsc --noEmit, prettier + rustfmt. I also ran a throwaway test asserting ErrorKind::ServiceRuntime.as_str() resolves to the right string in each of the five locales — rust_i18n falls back to echoing the key on a miss rather than failing the build, so a typo'd key would have shipped silently — then removed it.

The two questions from the description are still open whenever you get to them: whether the Embassy-v1 error-code should be preserved somewhere the OS won't read it as an ErrorKind, and whether the flat 10s retry wants bounding in the same release now that a failing service logs a stack on every attempt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants