Fix error serialization in 4 catch blocks where JSON.stringify(error) produces "{}"

[jeanschmidt]

Summary

Fix error serialization in 4 catch blocks where JSON.stringify(error) produces "{}" instead of the actual error message, making thrown errors useless for debugging.

Problem

JavaScript's Error object stores its message and stack properties as non-enumerable properties. JSON.stringify() only serializes enumerable properties, so:

const err = new Error("connection refused");
JSON.stringify(err);  // → "{}"

This means every error thrown from these 4 locations produces a message like:

cpToPod failed after 30 attempts: {}
execCpFromPod failed after 30 attempts: {}
job my-job has failed: {}
Pod my-pod is unhealthy with phase status Failed: {}

The actual error information — the message explaining what failed and why — is silently discarded. When these errors surface in CI logs, operators see that something failed 30 times but have no idea what the underlying error was. This makes debugging pod copy failures, job failures, and unhealthy pod conditions significantly harder than it needs to be.

Affected locations

All 4 are in packages/k8s/src/k8s/index.ts:

Function	Line	Context
execCpToPod	433	Thrown after 30 failed attempts to copy files into the job pod
execCpFromPod	530	Thrown after 30 failed attempts to copy files out of the job pod
waitForJobToComplete	577	Thrown when the K8s Job API watch reports a failure
waitForPodPhases	700	Thrown when a pod enters an unhealthy phase (Failed, Unknown)

These are all terminal error paths — the error is thrown up to the GitHub Actions runner, which logs it and fails the job. The error message is the only diagnostic information available to the workflow author.

Solution

Replace JSON.stringify(error) with:

error instanceof Error ? error.message : String(error)

This handles both cases:

• Error objects (the common case): extracts .message directly, producing readable output like "connection refused", "ETIMEOUT", "404 Not Found".
• Non-Error throwables (rare but possible — strings, numbers, objects thrown by dependencies): uses String() coercion, which calls .toString() and always produces something readable. This is strictly better than JSON.stringify() even for non-Error objects since String({}) → "[object Object]" while JSON.stringify({custom: "data"}) would work — but the Error case is what matters in practice.

After this fix

cpToPod failed after 30 attempts: connect ECONNREFUSED 10.0.0.5:443
execCpFromPod failed after 30 attempts: ETIMEOUT
job my-job has failed: Job reached backoff limit
Pod my-pod is unhealthy with phase status Failed: containers with unready status: [main]

What this does NOT change

• Error handling flow — same catch blocks, same retry counts, same throw-up-the-stack behavior
• The core.debug() calls earlier in each catch block (e.g. line 429: core.debug(`cpToPod: Attempt ${attempt + 1} failed: ${error}`)) — these already use template literal coercion which calls .toString() and works correctly. Only the final throw new Error(...) messages were broken.
• No new dependencies, no new error types, no behavioral changes

Files changed

• packages/k8s/src/k8s/index.ts — 4 lines changed (identical substitution in each)

Test plan

• npm run build succeeds (tsc + ncc)
• All 22 existing k8s-utils tests pass
• No remaining JSON.stringify(error) in the file
• In production for pytorch/* org that uses the fork https://github.com/jeanschmidt/runner-container-hooks

[Copilot]

Pull request overview

Fixes error serialization in the k8s package so thrown errors from key retry/terminal paths include meaningful messages instead of "{}", improving debuggability in CI logs.

Changes:

• Replace JSON.stringify(error) with error instanceof Error ? error.message : String(error) in 4 catch blocks.
• Add Jest tests to validate error messages for Error and non-Error throwables across affected functions.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
packages/k8s/src/k8s/index.ts	Updates 4 thrown error messages to properly serialize Error instances (and non-Error throwables).
packages/k8s/tests/error-serialization-test.ts	Adds regression tests covering the updated thrown error messages and retry exhaustion behavior.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

This wrap can produce duplicated messages when the underlying error already includes job ${jobName} has failed (e.g. from isJobSucceeded), resulting in job X has failed: job X has failed. Consider rethrowing the original error in that case, or changing the wrapper message to add new context (and/or use new Error(msg, { cause: error }) to preserve the original error).

Suggested change

	throw new Error(`job ${jobName} has failed: ${error instanceof Error ? error.message : String(error)}`)
	const jobFailedMessage = `job ${jobName} has failed`
	if (error instanceof Error && error.message.includes(jobFailedMessage)) {
	throw error
	}
	throw new Error(
	`${jobFailedMessage}: ${
	error instanceof Error ? error.message : String(error)
	}`
	)

[jeanschmidt]

Technically valid observation: isJobSucceeded throws "job X has failed", and the catch wraps it as "job X has failed: job X has failed". But this is pre-existing behavior — it was "job X has failed: {}" before. Our
PR just makes the duplication visible instead of hiding it behind {}. I'd say this is out of scope for a bug-fix PR — it's a separate improvement. The suggested fix also adds complexity (conditional rethrow) that goes beyond the intent of this change.

[Copilot]

When the pod enters an unhealthy phase, the inner throw already uses the same prefix (Pod ... is unhealthy with phase status ${phase}), so this wrapper will duplicate the message (...: Pod ...). Consider only wrapping errors that come from getPodPhase/backoff (and rethrowing the unhealthy-phase error as-is), or detect the already-prefixed message to avoid duplication. Also consider setting the original error as { cause: error } to preserve context.

[jeanschmidt]

Same reasoning. The inner throw at line 692 uses the same prefix, so the outer catch wraps it redundantly. Again pre-existing, just newly visible. Out of scope.