Skip to content

fix(artifact): bound image saves and recover failed publication - #89

Open
morluto wants to merge 7 commits into
scaleapi:mainfrom
morluto:codex/fix-artifact-publication
Open

morluto wants to merge 7 commits into
scaleapi:mainfrom
morluto:codex/fix-artifact-publication

Conversation

@morluto

@morluto morluto commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Closes #88

Apply one deadline to nonblocking reads from both Docker-save pipes, stream gzip output, and retain a bounded 16 KiB stderr tail. On failure, terminate the owned process group, close the parent pipe ends, and reap the save process. File and byte uploads now reuse the existing attempt-isolated locator scheme so a failed document insert cannot block the next publication.

Reproduced failure Before After
Stalled save with saturated stderr; 0.2 s deadline Still blocked at 2 s; external watchdog required Fails in 0.206 s
Retry after document insert fails Three retries collide with orphan object Next retry publishes version 1
Temporary archive after failed save Cleanup unreachable while blocked Removed
Retained stderr Unbounded Last 16 KiB; no stderr staging file
Detached child retains stdout/stderr Hang or cleanup failure Returns in 1.501 s at a 1.5 s deadline; archive removed

Measured with local subprocesses and real SQLite/filesystem stores. Tests cover stalled stdout, saturated stderr, surviving descendants, compression errors, valid 16 MiB gzip output, concurrent writes, and both file-upload APIs.

Only newly published object locators change. Existing saved locators still load. Failed publication can leave an isolated object; this change makes retry safe rather than adding garbage collection.

Sandbox file-method dispatch preserves providers implementing either the object-store method names or the deprecated S3 names, including inherited overrides and super() delegation. Regression tests cover artifact staging through an older provider and both keyword spellings.

Validation: 6,515 unit/protocol tests passed, 13 capability skips; A2A, sandbox, and protocol tests also pass after merging latest main. Plugin API check passed. Docker/cloud integration runs in CI.

RetriggerConfidence Score: 5/5

The PR appears safe to merge based on the reviewed changes.

What we checked:

  • Failed saves leave no archive: The save helper removes its output on failure, and the caller also removes its temporary file.
  • Upload fallback rereads the source: Each upload destination opens a fresh source, so a fallback starts reading at the beginning.

Summary

Docker image saves now stream under one deadline, and file uploads use a separate object location for each attempt so failed publications can be retried. Sandbox providers can keep using either the older or newer file-copy method names.

  • Docker image saves stream both pipes under one deadline.
  • Each file upload gets its own object location.
  • Sandbox providers can use either file-copy method name.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
  G[Staging grant] --> H{Staging path and port known?}
  H -- Yes --> L[Try agent server over loopback]
  L -- Connection refused --> P[Try grant URL]
  H -- No --> P
  L -- Server responds --> R[Use response]
  P --> R
Loading

Reviews (5) · Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." · Reviewed by Greptile

@morluto
morluto requested a review from a team as a code owner October 7, 2026 01:36
Comment thread src/agent_env/artifact/artifacts/docker_image.py Outdated
Comment thread src/agent_env/artifact/artifacts/docker_image.py Outdated
Comment thread src/agent_env/artifact/artifacts/docker_image.py Outdated
@morluto

morluto commented Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

Applied the shared sandbox-provider compatibility fix in c14168f, included in head ac4c1ed, and merged latest main. Either PR can merge first without dropping providers that override the deprecated file-transfer methods. The regression covers actual artifact staging, inherited overrides, VM super() delegation, and both keyword spellings.

Validation: 6,515 unit/protocol tests passed with 13 capability skips; after merging latest main, all 901 A2A/sandbox/protocol tests passed. Plugin API check passed. All existing inline review threads are resolved.

@earakely-scale

Copy link
Copy Markdown
Collaborator

🤖 Automated Claude routine — Env Pod PR Context Review. This is a bot, not Edgar Arakelyan; not a human review.

Context from outside this diff: open PR #85 carries the same two changes, byte for byte, and neither PR mentions the other. Both branch from main, so whichever merges first leaves the other conflicting on both files.

Both are currently blocked, and #85 already has a request on it to resolve conflicts, so this may already be in hand. If not, the usual seam is to land the shared compatibility shim plus its test once — in whichever of the two goes first — and rebase the other onto it, rather than resolving the same block twice.


Generated by Claude Code

This branch has not been deployed

No deployments
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.

fix(artifact): bound image saves and recover interrupted file publication

2 participants