fix(container-runtime): send the real error text as the JSON-RPC message - #3613
Open
JesseMarkowitz wants to merge 1 commit into
Open
Conversation
Both error paths in RpcListener set `message: typeof error`, which is the literal string "object" for every Error ever thrown. The JSON-RPC `message` field — the short description a caller reads first — therefore carried no information on any container-runtime failure, and the actual text was reachable only by digging into `data.details`. This is what makes a failing service opaque. When a package's `main` rejects, `SystemForStartOs.start` has no catch, so the rejection travels through `handleRpc` to the host, which retries every SYNC_RETRY_COOLDOWN_SECONDS. The operator sees a service restarting on a timer, and the one field carrying a reason says "object". Diagnosing it means reading the package source and inferring which throw fired. `message` now carries `error.message` when present and `String(error)` otherwise; `data.details` and `data.debug` are untouched, so nothing that reads them changes. Nothing keys off the previous value. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TJE2aaFnjtxseNg2pxPUTx
Author
|
Issue found by Claude while I was working on the Vikunja service... |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Both error paths in
RpcListenerset the JSON-RPC errormessagetotypeof error:typeof erroris the literal string"object"for everyErrorever thrown. So the field a caller reads first — the JSON-RPC spec's "short description of the error" — carries no information on any container-runtime failure, and the real text is reachable only by digging intodata.details. Note thatmapErroralready computes the right expression one line below, fordetails.Why this is worth fixing beyond tidiness
It is a meaningful part of why a failing service is opaque to diagnose.
When a package's
mainrejects,SystemForStartOs.starthas nocatch, so the rejection travels out throughhandleRpcto the host, and the service actor retries everySYNC_RETRY_COOLDOWN_SECONDS. What the operator sees is a service restarting on a fixed timer, and the one field that should say why saysobject.I hit this on a real package. A service restart-looped every 10s for hours; the service log showed only the package's own pre-throw output, the OS log had nothing I could match, and
statusInfo.errorwasnull. Every one of that package's actions worked fine — onlymainfailed, and onlymainwas silent. Diagnosing it in the end meant reading the package source and reasoning about whichthrowcould fire. The cause was a one-line guard in the package.The change
messagenow carrieserror.messagewhen present,String(error)otherwise.data.detailsanddata.debugare untouched, so anything already reading those is unaffected. I checked that nothing on the Rust side keys off the previous value.On the changelog and version
Deliberately not included, and I'd like guidance.
CHANGELOG.md's top heading is## [0.4.0.1], andstart-os/v0.4.0.1is a cut tag — so per the rootAGENTS.mdrule this entry belongs under a new heading, which pairs with a manifest bump. For StartOS that means rootpackage.json, theCargo.tomllabel,Cargo.lock, and a newversion/v0_4_0_2.rsmigration module. Creating an OS version module defines the upgrade path, and that seemed like a release decision to leave to maintainers rather than presume in a one-line fix.Happy to add it in whatever form you prefer. Suggested text:
Related
There is a second, larger half to this that I have not touched, because the fix shape is a design decision:
service_actor.rsalready carries// TODO: ideally this error should be sent to service logson the line that swallows this, andlog_err()routes to the OS log rather than the service log — which is where an operator actually looks. Filed separately as #3614 rather than guessed at here.Verification
make container-runtime-test— 2 suites, 12 tests, 6 snapshots, all passnpm --prefix projects/start-os/container-runtime run check— cleanprettier --checkon the touched file — clean🤖 Generated with Claude Code
https://claude.ai/code/session_01TJE2aaFnjtxseNg2pxPUTx