Repository navigation
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if file_path is not None: | ||
| return store.put_object_file( | ||
| artifact_type="file", | ||
| id=id, | ||
| version=version, | ||
| object_name=filename, | ||
| file_path=file_path, | ||
| content_type=content_type, |
There was a problem hiding this comment.
Two concurrent FileArtifact.put() calls can still choose the same version and pass the existence check in S3ObjectStore._put_file() before either upload finishes. That method uploads without a create-only condition, so neither call raises ObjectAlreadyExistsError and the new fallback never runs. Both documents can publish with the same object_url, leaving one artifact with the other writer's contents.
This leaves the concurrency fix incomplete. Use an attempt-specific path before uploading files, or make S3 file creation atomic.
Knowledge Base Used:
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agent_env/artifact/artifacts/file.py
Line: 143-150
Comment:
**Concurrent S3 uploads overwrite files**
Two concurrent `FileArtifact.put()` calls can still choose the same version and pass the existence check in `S3ObjectStore._put_file()` before either upload finishes. That method uploads without a create-only condition, so neither call raises `ObjectAlreadyExistsError` and the new fallback never runs. Both documents can publish with the same `object_url`, leaving one artifact with the other writer's contents.
This leaves the concurrency fix incomplete. Use an attempt-specific path before uploading files, or make S3 file creation atomic.
**Knowledge Base Used:**
- [Artifact management and delivery](https://app.greptile.com/scale-ai/-/custom-context/knowledge-base/scaleapi/agentenv-framework/-/docs/artifact-management.md)
- [Storage, objects, documents, and secrets](https://app.greptile.com/scale-ai/-/custom-context/knowledge-base/scaleapi/agentenv-framework/-/docs/storage-and-secrets.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| # (which is also what unblocks a read stuck on a stalled process's stdout). | ||
| save_proc.wait() | ||
| timer.cancel() | ||
| drainer.join(timeout=5) |
There was a problem hiding this comment.
The new drainer.join(timeout=5) uses an unnamed five-second limit. The repository requires magic numbers to be stored as descriptive class or instance variables. The new stderr read size has the same issue, and the new module-level limits also do not follow that requirement.
Put these settings on the relevant classes and reference them from the helpers. This repository requirement must be satisfied before merging.
Rule Used: Store magic numbers as class or instance variables with descriptive names rather than using them inline in the code. (source)
Learned From
scaleapi/scaleapi#126388
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agent_env/artifact/artifacts/docker_image.py
Line: 650
Comment:
**Numeric settings break repository rules**
The new `drainer.join(timeout=5)` uses an unnamed five-second limit. The repository requires magic numbers to be stored as descriptive class or instance variables. The new stderr read size has the same issue, and the new module-level limits also do not follow that requirement.
Put these settings on the relevant classes and reference them from the helpers. This repository requirement must be satisfied before merging.
**Rule Used:** Store magic numbers as class or instance variables with descriptive names rather than using them inline in the code. ([source](https://app.greptile.com/scale-ai/-/custom-context?memory=002e0051-41ad-46c1-9098-47433c580150))
**Learned From**
[scaleapi/scaleapi#126388](https://github.com/scaleapi/scaleapi/pull/126388)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
🤖 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 #89 also claims issue #88, and touches the same two files.
Only offering the cross-PR context; no opinion on which approach is right. Generated by Claude Code |
Summary
Fixes #88. Artifact publication had two failure paths:
DockerImageArtifact.put()read thedocker saveprocess's stdout to EOF before waiting on it, and never drained stderr. A save whose stderr filled the pipe deadlocked, and a stalled save blocked the read itself, so the timeout — applied only in thewait()after EOF — never fired.FileArtifact.put()/put_bytes()upload to the write-once path of the version they reserve, before the document is inserted. When the insert failed, the orphaned object stayed on that path and every retry chose the same version and collided with it (ObjectAlreadyExistsError); two writers reserving the same version collided the same way.Changes
docker_image.py: stream the save through a new_stream_save_outputhelper. stderr drains on a thread into a bounded 64 KiB tail (kept for the failure message), and a watchdog timer kills the process atDOCKER_SAVE_TIMEOUT_SECONDSinstead of timing out only the post-EOF wait. The process is reaped on every path, stdout is still streamed into the gzip file chunk by chunk (archive memory stays bounded), and the existing timeout/failure error messages are unchanged.file.py:put()andput_bytes()now upload through a shared_upload. The happy path is unchanged — the object still lands on the reserved version's path, byte-identical to before. On a collision,_uploadchecks whether that version was actually published: if a concurrent writer won the reservation, it reserves the next version and retries (bounded); if no document exists for the version, the object is an orphan from a failed attempt, and since write-once objects are never deleted or overwritten, the upload falls back to a prefix of this attempt's own (ArtifactStore.attempt_prefix, the mechanismput_attemptandDockerImageArtifact.putalready use). Published objects are never overwritten.One honest limitation: an orphaned object left by a failed attempt is not garbage-collected (the object store has no delete); it is simply no longer in anyone's way.
Testing
New regression tests (all written against the failure modes above and confirmed to fail or hang on the unpatched code via a standalone reproduction):
tst/unit/artifact/test_file_artifact_attempts.py— retry after a simulated document-insert failure succeeds for bothputandput_bytes; two concurrent uploads whose uploads are barrier-synchronized both publish, with distinct versions/URLs and intact contents; a second put never overwrites the first version's object.tst/unit/artifact/test_docker_image_save.py— against a PATH-stubbeddocker: a save flooding stderr with 300 KB completes and stores the archive; a stalled save is killed at a 2-second timeout with the existing "timed out" error; a failing save reports its stderr; the happy path stores the streamed archive (verified by decompressing whatload()returns).Results:
tst/unit/artifact+tst/unit/store+tst/unit/bundle: 2538 passed, 10 skipped, 33 failed — all 33 failures are intst/unit/store/local_grant_server_test.pyand also fail on unmodifiedmainin this environment (they need a loopback HTTP server this sandbox blocks), so they are unrelated to this change.ruff checkon the changed files: clean.Fix the remaining S3 overwrite path and satisfy the numeric-settings rule before merging.
Fix with agent prompt
Summary
Adds a watchdog and bounded stderr capture for
docker save, plus collision recovery for file uploads.Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[File upload] --> B[Choose next version] B --> C[Upload to version path] C -->|Success| D[Insert artifact document] C -->|Collision| E{Document exists?} E -->|Yes| B E -->|No| F[Upload to unique attempt path] F --> D C -.-> G[S3 file writes can overwrite without a collision error]Reviews (1) · Last reviewed commit: "fix(artifact): bound docker save and iso..." · Reviewed by Greptile