fix(bench): Remove bench image after archival (backport #508)#530
Conversation
(cherry picked from commit c28a4f1)
|
| Filename | Overview |
|---|---|
| agent/server.py | Adds registry manifest check before local image removal on bench archival; logic is sound but exceptions are fully suppressed with no logging, and one guard check is unreachable. |
Sequence Diagram
sequenceDiagram
participant AB as archive_bench()
participant RDI as remove_docker_image()
participant DIM as docker_inspect_manifest()
participant Docker as Docker CLI
AB->>AB: resolve image_tag from Bench
AB->>RDI: remove_docker_image(image_tag)
note over RDI: contextlib.suppress(Exception)
RDI->>DIM: docker_inspect_manifest(image_tag)
DIM->>Docker: docker manifest inspect tag
alt Image found in registry
Docker-->>DIM: manifest JSON
DIM-->>RDI: manifest (truthy)
RDI->>Docker: docker rmi tag --force
Docker-->>RDI: success
else no such manifest
Docker-->>DIM: error output
DIM--xRDI: raise Exception(not found)
note over RDI: suppressed silently
else Other error (network, auth)
Docker-->>DIM: error
DIM--xRDI: re-raise AgentException
note over RDI: also suppressed silently
end
Reviews (1): Last reviewed commit: "fix(bench): Remove bench image after arc..." | Re-trigger Greptile
| with contextlib.suppress(Exception): | ||
| manifest = self.docker_inspect_manifest(image_tag) | ||
| if not manifest: | ||
| return | ||
| self.execute(f"docker rmi {image_tag} --force") |
There was a problem hiding this comment.
Silent exception swallows both "not in registry" and real errors
contextlib.suppress(Exception) catches every exception thrown inside the block — including transient network failures, Docker daemon errors, and authentication problems. When docker_inspect_manifest raises because the registry is temporarily unreachable (the AgentException re-raise path), the image is silently left on disk with no log entry and no way to tell the difference from the intentional "image not in registry, skip removal" case. Adding at least a logger.warning before suppressing would make failures observable without changing the best-effort semantics.
| manifest = self.docker_inspect_manifest(image_tag) | ||
| if not manifest: | ||
| return |
There was a problem hiding this comment.
if not manifest: return is unreachable dead code
docker_inspect_manifest never returns a falsy value on success — if docker manifest inspect exits cleanly it emits a non-empty JSON blob; if the image is absent it raises Exception("Image ... not found in registry"); for all other failures it re-raises AgentException. None of those paths produce a falsy return, so the early-return guard will never fire. The guard can be removed to avoid confusion about why the subsequent docker rmi might be skipped.
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!
Check if the image is there on registry.
If it's available, remove from local.
This is an automatic backport of pull request #508 done by Mergify.