fix(copilot): let a raw container declare a permanent failure

[mitja-kleider]

Tracking issue

No upstream issue. Related to #7922, which reconnected the error document to the reader
and listed this as deliberately out of scope:

Sidecar writes every uploader failure as RECOVERABLE with Origin unset, so a
container's own _ERROR file comes back retryable rather than a permanent USER error.
Pre-existing, but this PR is what makes it observable.

This is a follow-up adding the permanent USER error.

Why are the changes needed?

A raw container has two ways to report failure and neither can express a permanent user
error.

Exiting non-zero is classified as a system failure: the task's retries is never
consulted and the node is retried up to max-node-retries-system-failures instead.

Writing the error file is the documented alternative, and RecursiveUpload picks it up —
but Sidecar then discards what the container said about it, rewriting every uploader
failure as OutputUploadFailed / RECOVERABLE / Origin unset. A container that knows
its failure is deterministic has no way to say so, and retries cannot express it either:
the whole point of retries=0 is defeated by a system-failure budget the task does not
control.

Today the only workaround for us is the container speaking flyte's blob protocol itself:
build an ErrorDocument, construct an S3 PUT and write error.pb into the output
prefix.

What changes were proposed in this pull request?

Let the container write a serialized core.ErrorDocument to its error file and forward it
untouched, so the Kind and Origin it chose are what flyte acts on.

1. flytecopilot/data/upload.go — when the error file's bytes are an ErrorDocument,
   RecursiveUpload returns the new ContainerError carrying it. A document is
   recognised by structure, a parse that yields a code or a message, rather than by
   re-encoding to the same bytes: the wire format is not canonical, so field order,
   varint width and a field a newer idl added all differ legitimately between encoders.
   proto.Unmarshal does accept some plain text by chance, but such a parse leaves the
   error's own fields empty, and a document with neither a code nor a message says
   nothing the plain-text path cannot.
2. flytecopilot/cmd/root.go — UploadErrorDocument writes a caller-built document;
   UploadError keeps its signature and its recoverable classification, delegating.
3. flytecopilot/cmd/sidecar.go — Sidecar forwards a ContainerError's document and
   classifies everything else exactly as before.

Backwards compatible by construction: an error file that is not a document still becomes
User Error: <contents> under OutputUploadFailed / RECOVERABLE.

Two adjacent things left alone deliberately:

• A plain-text error file arguably should not be recoverable either (a container that
  wrote its error and exited is done deciding), and its Origin is arguably USER rather
  than unset. Both change existing behaviour for anyone relying on the retry, so they are
  not in here.
• The _ERROR protocol is not documented for raw-container authors as far as I can find.
  If there is a page for it, I'll document the document form there.

How was this patch tested?

New tests, each watched fail before the fix and pass after:

• TestUploader_RecursiveUpload_ContainerErrorDocument — a document in the error file
  comes back as ContainerError with the document intact. Before the fix it fails showing
  exactly today's behaviour, the serialized proto stringified into a message:
  in chain: "User Error: \n2\n\x0fBenchmarkFailed\x12\x1dthe model rejected the prompt \x01".
• TestUploader_RecursiveUpload_ContainerErrorDocumentFromAnotherEncoder — the same
  document hand-encoded with the fields out of order and carrying a field this binary
  does not know is still read as a document, with the container's code, message and
  Origin intact.
• TestUploader_RecursiveUpload_ContainerErrorMessage — a plain-text error file still
  produces User Error: failed and no ContainerError. Guards the compatibility claim;
  passes before and after.
• TestSidecarForwardsTheContainersErrorDocument — end to end across the two components:
  Sidecar runs against a memory store, then NewRemoteFileOutputReader over the same
  prefix must report IsError(), IsRecoverable == false, Kind == USER and the
  container's own code and message. Before the fix: expected BenchmarkFailed, actual OutputUploadFailed.

go build ./flytecopilot/...
go test ./flytecopilot/... ./flyteplugins/go/tasks/pluginmachinery/ioutils/...

All pass, gofmt and go vet clean.

Labels

fixed

Check all the applicable boxes

• I updated the documentation accordingly. (code comments; see the note above about
  raw-container docs)
• All new and existing tests passed.
• All commits are signed-off.

[Copilot]

🟡 Changes recommended

The current byte-for-byte “re-encode to self” protobuf check can reject valid ErrorDocument encodings, causing container-declared permanent errors to be misclassified as plain-text retryable errors.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR improves raw-container failure reporting in flytecopilot by allowing a container to write a serialized core.ErrorDocument to its local error file and have that document propagated to remote storage without being reclassified by the sidecar, preserving the container-chosen Kind and Origin.

Changes:

• Detect core.ErrorDocument content in the container error file during RecursiveUpload and surface it as a new ContainerError.
• Add UploadErrorDocument so callers can upload an explicitly constructed error document (while keeping UploadError behavior unchanged).
• Update Sidecar to forward container-declared error documents untouched; add end-to-end tests covering both proto and plain-text error file behavior.

File summaries

File	Description
flytecopilot/data/upload.go	Detects and returns a ContainerError when the error file contains an ErrorDocument.
flytecopilot/data/upload_test.go	Adds unit tests for proto-document vs plain-text error file handling.
flytecopilot/cmd/sidecar.go	Forwards ContainerError documents unchanged; preserves prior classification for uploader failures.
flytecopilot/cmd/sidecar_test.go	Adds an end-to-end test verifying the container’s error document is what the output reader observes.
flytecopilot/cmd/root.go	Introduces UploadErrorDocument and makes UploadError delegate to it.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.