Repository navigation
fix(OMN-16367): resolve in-use images by digest, and give the dry run a citable number - #3757
Conversation
… a number Three follow-ups to #3755, one of which is a real safety defect in the shipped plan. 1. IN-USE RESOLUTION BY DIGEST. disk-gc.sh built its plan-time in-use set from `docker ps --all --format '{{.Image}}'`. That field prints the image NAME for a name-referenced container, so cross-referencing on it returned a false zero on 29 of 30 rows on this host. Measured against a read-only capture of the live inventory (795 images, 61 distinct container image digests), the name-built plan selected an image a running container was sitting on -- a dangling postgres layer, 611 MB -- and the digest-built plan selected zero. The execution-time ancestor re-check would very likely have caught that one, but "a second check probably saves us" is a works-by-convention surface, and the plan and its dry-run total are wrong in the meantime. It matters most where it is least visible: the stability-test, judge and lakshman lanes run containers on digests OLDER than their own `:latest`, so a name-based check protects the tag while the running layer goes unprotected. The name list is still unioned in. It can only ADD protection -- a stale name ref keeps an image that might not have needed keeping, the safe direction -- but it is never the sole source. 2. A CITABLE DRY-RUN TOTAL. The plan now carries `remove_nominal_bytes` and the dry run prints the image count and GB it would remove, so a host-rollout consent row can quote a number instead of a hand wave. Both the log line and the code comment say NOMINAL, not reclaim: shared base layers collapse, and a measured pass on this host recovered 17.6% of nominal. 3. TAG SHAPES ARE CONFIG, NOT CODE. The build-stamped shapes move to `generation_tag_patterns` in the keep-list, with the same five shapes as defaults. Widening what counts as disposable is now a config review. An unparseable pattern is skipped rather than raised: a broken config must not turn a GC into a crash, and skipping lands on KEEP. Re-proved against the same live capture with digest exclusion: 327 images, 298.5 GB nominal, and all four safety readbacks zero -- nothing referenced by digest, nothing referenced by repo:tag, no keep-tag or moving pointer, no governed-lane image. Tests RED-first: 10 of 12 failed against the unchanged code. 85 disk-gc tests pass.
#10212) * evidence: OCC companion pass 1 for OmniNode-ai/omnibase_infra#3757 * evidence: OCC companion self-bind for #10212 --------- Co-authored-by: node-occ-companion-effect <occ-companion-effect@omninode.ai>
|
| Surface | Meaning | Blocks merge? |
|---|---|---|
| Review threads | Per-finding, posted by the reviewer | No (informational) |
Hostile Review Thread Gate |
Deterministic: unresolved hostile-reviewer threads exist | Fails until resolved (not yet a required context) |
degraded verdict |
Fewer than 2 models succeeded (infra) | No |
Powered by omniintelligence.review_pairing.cli_review — multi-model adversarial review: qwen3-review, qwen3-review-b, glm-review (OMN-8468/OMN-8524/OMN-17492)
There was a problem hiding this comment.
Hostile Reviewer — adversarial findings (OMN-17492)
Models succeeded: glm-review
Models failed: codex
New finding threads: 0
Deduped (already posted on this PR): 0
Below quorum (one model only, reported not threaded): 8
Nit-level findings suppressed: 1
The model is the FINDER, never the gate: merge is gated only by the
deterministic Hostile Review Thread Gate, which blocks while
hostile-reviewer threads are unresolved. Resolve each thread after
addressing (or rejecting, with a reply) its finding.
Below quorum: 8 finding(s) raised by one model only (OMN-18479)
These are reported and NOT dropped, but they get no thread and do not block: a single model's finding no other model reproduced is not evidence enough to stop a merge. Read them; act on them if they are right.
- [MAJOR]
scripts/disk-gc.sh, dry-run block (~line 145)(glm-review) — Shell-to-Python interpolation of planner value enables code injection | The new GB formatting line embeds REMOVE_NOMINAL_BYTES directly into a python3 -c program via unquoted shell expansion inside do - [MAJOR]
scripts/disk-gc.sh, in-use resolution block (~line 78)(glm-review) — N+1 docker inspect loop for in-use digest resolution | The fix replaces one docker ps invocation with a shell loop spawning one docker inspect subprocess per container. The PR's own test fixture speak - [MINOR]
scripts/disk-gc.sh, REMOVE_NOMINAL_BYTES extraction(glm-review) — null remove_nominal_bytes bypasses the .get default | The shell extraction uses .get("remove_nominal_bytes", 0), which only covers key absence. If the planner emits an explicit null (json.load maps it - [MINOR]
scripts/disk_gc_plan.py / deploy/disk-gc/keep-list.yaml(glm-review) — Duplicate pattern lists invite silent config/code drift | The five regexes are now maintained in two places: DEFAULT_GENERATION_TAG_PATTERNS in disk_gc_plan.py and generation_tag_patterns in keep-list - [MINOR]
scripts/disk_gc_plan.py, _parse_size(glm-review) — _parse_size is untested despite handling dirty input | The new _parse_size function implements a size grammar, unit table, and three failure paths (no match, unknown unit mapping to 0, float conversio - [MINOR]
scripts/disk_gc_plan.py, _parse_size(glm-review) — Unknown regex units silently zeroed in _parse_size | _SIZE_UNITS.get(m.group(2).upper(), 0) maps any unrecognized unit to 0, so a size string like '3.66PB' or a locale-variant unit produces a silent z - [MINOR]
scripts/disk_gc_plan.py, _is_build_stamped_tag(glm-review) — Malicious/typo'd regex patterns silently disable the generation rule | The new pattern-driven _is_build_stamped_tag skips re.error with a bare continue and no logging. A config typo (unclosed bracket, - [MINOR]
tests/unit/scripts/test_disk_gc_digest_inuse_and_plan_totals.py, TestDigestOnlyReferenceIsHonoured(glm-review) — Script test asserts inspect was called but not that digest output is consumed | test_script_resolves_in_use_by_inspecting_container_image_digests only asserts that a docker inspect with '.Image' in it
Three follow-ups to #3755. One is a real safety defect in the plan that shipped.
1. In-use resolution by digest
disk-gc.shbuilt its plan-time in-use set from:That field prints the image NAME for a name-referenced container and the id only for an id-referenced one. Cross-referencing on it returned a false zero on 29 of 30 rows on this host, and the
201-data-breakdown-1045report re-derived its whole inventory by digest for exactly this reason.Measured against a read-only capture of the live inventory (795 images, 61 distinct container image digests):
{{.Image}}namespostgreslayer, 611 MB.ImagedigestsThe execution-time
ancestorre-check would very likely have caught that one, but "a second check probably saves us" is a works-by-convention surface, and the plan and its dry-run total are wrong in the meantime. The dry-run total is what a host-rollout consent row cites, so a plan that over-reports is a governance problem, not only a hygiene one.It matters most where it is least visible. The stability-test, judge and lakshman lanes run containers on digests older than their own
:latest, so a name-based check protects the tag while the running layer goes unprotected. Digest exclusion is the only sound check there.The name list is still unioned in. It can only add protection — a stale name ref keeps an image that might not have needed keeping, which is the safe direction — but it is never the sole source.
2. A citable dry-run total
The plan now carries
remove_nominal_bytes, and the dry run prints the image count and GB it would remove, so a consent row can quote a number instead of a hand wave.Both the log line and the code comment say NOMINAL, not reclaim: shared base layers collapse, and a measured pass on this host recovered 17.6% of nominal. The wording exists so nobody quotes the big number as expected disk returned.
3. Tag shapes are config, not code
The build-stamped shapes move to
generation_tag_patternsin the committed keep-list, with the same five shapes as defaults. Widening what counts as disposable is now a config review.An unparseable pattern is skipped rather than raised: a broken config must not turn a scheduled GC into a crash, and skipping lands on KEEP. A test pins that the shipped patterns still cover every shape measured on the host, and that
dev,main,latest,v1.2.3andrelease-2026match none of them.Re-proved against the same live capture
With digest exclusion, shipped keep-list:
Attribution: 54 images / 197.3 GB
onex-lab/omninode-runtime, 75 / 85.8 GBonex-lab/omnicloud-core, 24 / 27.5 GB ECRomnicloud/core, 13 / 4.5 GB ECRomniweb, plus the migrate families.Tests
RED-first: 10 of 12 failed against the unchanged code. 85 disk-gc tests pass. Full pre-commit exit 0,
bash -nclean, zero bypass flags.One test asserts the script actually runs
docker inspect -f '{{.Image}}', against a fake docker that records argv — so a revert to the name-printing field alone is a red test rather than a review catch.Not in this PR
The one-time removal of the existing backlog is a separate lane under an operator consent row. This sweeper owns the future. Host rollout is unchanged and still the operator's: the timer runs the script from a host clone in detached HEAD, and the dry-run path now prints the exact count and total to cite.
Evidence-Ticket: OMN-16367
Evidence-Source: OCC#10212