Repository navigation
fix(OMN-16367): bound the per-sha image families that actually hold the .201 disk - #3755
Conversation
…he disk Neither suspected cause of the .201 bleed survived measurement, and this lands the fix for what did. Not build cache. Three approved `docker builder prune` runs reclaimed 0B, the last one carrying `--all`. `docker buildx du` reads Shared 723.5GB of Total 750GB, so ~96% of the cache is content shared with image layers in the containerd snapshot store, which no builder prune can return while those images exist. On this buildx `-a, --all` means "include internal/frontend images", not the classic CLI's "remove all unused build cache" -- a scope modifier that reads like a force flag. The size cap merged in #3754 bounds future growth and recovers ~nothing today; its `--all` comment claimed otherwise and is corrected here, in the script and in its test. Not CI proof images. Exactly 3 proof-pattern images exist across 792 on the host. The wave-1 `--rmi local` teardown works, because those compose services declare no `image:` field, so `--rmi local` genuinely removes what they build. The 1227 stranded proof images the ticket was opened about all predate that fix and were evicted this morning. What actually accumulates is the deploy agent's per-sha tags, rebuilt roughly hourly and never reaped -- counted live: 60 images 219.3 GB onex-lab/omninode-runtime 179 images 205.1 GB ECR omnicloud/core 93 images 106.3 GB onex-lab/omnicloud-core Two planner defects let that happen: 1. A repo matching no `keep_image_repos` substring fell through to a conservative permanent KEEP. omnicloud/core, omnicloud-core, omninode-cloud-migrate and omniweb all miss that list, so ~408 images and 350+ GB grew without bound. 2. Repos that did match were still gated behind `min_age_days: 3`. At an hourly cadence ~72 generations are always too young for a keep-newest-N window to bite. The 60 live generations of onex-lab/omninode-runtime are that arithmetic. The fix is this ticket's own DoD item 2: a keep-last-N window over per-sha tag families, excluding anything referenced by a container, never a blanket prune. Three new keep-list values -- `generation_bounded_repos`, `generation_keep: 3`, `generation_min_age_hours: 6`. The floor is in hours, not days, which is the whole point of defect 2. Safety is opt-in twice over. A repo nobody listed is untouched, and a tag that is not build-stamped -- `dev`, `main`, a release name, anything in `keep_image_tags` -- is untouched. Both failure directions land on KEEP. The governed stability, judge and lakshman lanes are held out of scope structurally: their repo names match no entry. `protect_running` and the execution-time ancestor re-check are unchanged and still run first. Every tag shape the matcher recognises was read off the host, not invented. The loose `<word>-<hex>` shape is deliberately NOT matched: it is a per-sha tag too, but a pattern that broad would also catch ordinary meaningful tags, and an unmatched tag simply means KEEP. Tests RED-first: 9 of 20 failed against the unchanged planner, including both defects and every tag shape. 73 disk-gc tests pass.
…ibase_infra#3755 (#10203) * evidence(OMN-16367): author OCC companion for OmniNode-ai/omnibase_infra#3755 OCC companion by node_pr_lifecycle_fix_effect (OMN-13317 F1 / OMN-13990 / OMN-14285). Product PR head d8a850f4a8bd0bdc3ed36564ebe8b3e8f9bce45e. * evidence(OMN-16367): self-bind OCC#10203 + rebind contract_sha256 --------- Co-authored-by: omnimarket-bot <bot@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): 7
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: 7 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.
- [CRITICAL]
scripts/disk_gc_plan.py, generation reaping loop(glm-review) — Multi-tag images can be reaped by ID, destroying kept rollback pointers | The planner operates on docker image rows, which are (repo, tag) pairs; a single image ID frequently carries multiple tags. If - [MAJOR]
scripts/disk_gc_plan.py build_plan; deploy/disk-gc/keep-list.yaml(glm-review) — generation_keep accepts invalid values; 'never below 2' is documentation only | The keep-list comment states generation_keep is 'Never below 2' because the deploy agent needs a rollback target, and th - [MAJOR]
tests/unit/scripts/test_disk_gc_generation_bounded_repos.py(glm-review) — No test covers the same-ID multi-tag case the reaping logic is most exposed to | Every test constructs images with unique IDs and a single tag each. The realistic failure mode for this feature (one im - [MAJOR]
scripts/disk_gc_plan.py, generation reaping loop(glm-review) — Age-based ordering assumes CreatedAt parse order is generation order; ties and clock skew unhandled | generations_by_repo sorts by parsed age and slices the first generation_keep as survivors. Docker - [MINOR]
scripts/disk_gc_plan.py, _is_generation_bounded_repo(glm-review) — Substring repo matching over-matches crafted or coincidental repo names | _is_generation_bounded_repo uses unanchored substring matching ('same convention as keep_image_repos'). The pattern 'onex-lab/ - [MINOR]
scripts/disk_gc_plan.py, generation-bounded guard condition(glm-review) — Dangling and -tagged siblings of bounded families are excluded from reaping | The generation path requires a non- tag. Hourly rebuilds typically also produce dangling : rows (u - [MINOR]
scripts/disk_gc_plan.py, generation-bounded block(glm-review) — Plan-then-execute race window widened by bypassing min_age_days | The plan is computed from a snapshot and executed later. By moving reaping ahead of the day-scale age gate, images as young as 6 hours
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): 7
Nit-level findings suppressed: 2
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: 7 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_plan.py, generation-bounded block in build_plan(glm-review) — Tag-shape safety guarantee breaks for images carrying multiple tags | The planner's safety story is 'a tag that is not build-stamped is untouched.' Docker image listings key by image ID, and one image - [MAJOR]
scripts/disk_gc_plan.py, _PREFIXED_SHA_TAG_RE and tests/unit/scripts/test_disk_gc_generation_bounded_repos.py TestTagShapeOptIn(glm-review) — Moving pointers matching build-stamped shapes are reapable | The docstring asserts moving pointers (dev,main) fail the shape check and are never reaped. That holds only for baredev/main. If - [MAJOR]
scripts/disk_gc_plan.py, build_plan config parsing(glm-review) — No runtime validation of generation_keep or generation_min_age_hours | The invariant 'generation_keep: never below 2' is asserted only against the shipped YAML in a unit test. build_plan accepts any i - [MINOR]
tests/unit/scripts/test_disk_gc_generation_bounded_repos.py, TestExistingSafetyInvariantsSurvive(glm-review) — In-use protection test bypasses the containers code path | test_in_use_image_is_never_reaped passes containers=[] and protection only via inuse_refs. If build_plan derives protected images from the co - [MINOR]
scripts/disk_gc_plan.py, generation window sort(glm-review) — Age-tie sorting can reap the wrong generation | Docker CreatedAt has second resolution. Multiple generations built in the same second (or clock skew across build hosts) produce equal age keys; the sor - [MINOR]
scripts/disk_gc_plan.py, generation-bounded guard clause(glm-review) — Dangling images in bounded repos have no explicit handling | The rule requires both repo and tag to be non-; a dangling image (repo or tag '') in a bounded repo falls through past the gene - [MINOR]
tests/unit/scripts/test_disk_gc_generation_bounded_repos.py, TestShippedKeepListEnablesTheMeasuredFamilies(glm-review) — keep-list and shipped-config test omit omninode-cloud-migrate | The keep-list comment names four growing families including omninode-cloud-migrate, but TestShippedKeepListEnablesTheMeasuredFamilies ch
… a number (#3757) 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.
Closes DoD item 2 of OMN-16367. Follows
omnibase_infra#3754, and corrects a factual claim that PR made.Neither suspected cause survived measurement
Not build cache. Three approved
docker builder pruneruns reclaimed 0B, the last one carrying--all(ledger rows 3921, 3932, 4054-4060).docker buildx duright now reads Shared 723.5 GB of Total 750 GB — ~96% of the cache is content shared with image layers in the containerd snapshot store, which no builder prune can return while those images exist.On this buildx
-a, --allis documented as "Include internal/frontend images", not the classic CLI's "remove all unused build cache". It is a scope modifier that reads like a force flag. #3754's comment claimed--allwas load-bearing because a default prune excludes image-referenced records. That was wrong, and this PR corrects it inscripts/disk-gc.shand in its test. The size cap still bounds future cache growth; it recovers ~nothing on this host, and the code no longer oversells it.Not CI proof images. Exactly 3 proof-pattern images exist across 792 on the host. The wave-1
--rmi localteardown from #3121 works, and it works for a concrete reason: those compose services declare noimage:field, so--rmi localgenuinely removes what they build. The 1227 stranded proof images the ticket was opened about all predate that fix and were evicted this morning by the 0700 lane. A teardown change or a proof-tag sweeper would be built against a class that produces 3 images.What actually accumulates
The deploy agent's per-sha tags, rebuilt roughly hourly and never reaped. Counted live on
.201:onex-lab/omninode-runtimeomnicloud/coreonex-lab/omnicloud-coreplus
omninode-cloud-migrate(96 + 92),omnibase-infra-migrate(60) and ECRomniweb(40).Two planner defects let that happen.
keep_image_repossubstring falls through to"repo not in keep_image_repos (conservative keep)".omnicloud/core,omnicloud-core,omninode-cloud-migrateandomniweball miss that list, so ~408 images and 350+ GB grow without bound.min_age_daysdefeats a keep-newest-N window at an hourly cadence. Repos that do match are still gated behindmin_age_days: 3, so ~72 generations are always too young before "keep the newest 2" can bite. The 60 live generations ofonex-lab/omninode-runtimeare that arithmetic.The fix is this ticket's own DoD item 2
Three new keep-list values:
generation_bounded_reposgeneration_keep3generation_min_age_hours6Safety is opt-in twice over
A repo nobody listed is untouched. A tag that is not build-stamped —
dev,main, a release name, anything inkeep_image_tags— is untouched. Both failure directions land on KEEP.protect_runningand the execution-timeancestorre-check are unchanged and still run before this branch, so an in-use image is unreachable by it.The governed lanes are held out structurally, not by a name check.
omnibase-infra-stability-test-*matches no entry ingeneration_bounded_repos, so stability, judge and lakshman are out of scope by construction. A test pins that, using their realpreflight-<stamp>tags — which look build-stamped and must still be kept.Every tag shape the matcher recognises was read off the host, not invented:
The loose
<word>-<hex>shape (main-f3d4029,runtime-fc61f41242dcc01b) is deliberately not matched. It is a per-sha tag too, but a pattern that broad would also catch ordinary meaningful tags, and an unmatched tag simply means KEEP. Narrow it in, never out.Tests — RED-first
9 of 20 failed against the unchanged planner, covering both defects and every tag shape. 73 disk-gc tests pass. Full pre-commit exit 0,
bash -nclean, zero bypass flags.The exposed-identifier gate correctly rejected the real ECR account id in a fixture; it is replaced with a synthetic stand-in rather than annotated, since only the repo suffix is load-bearing.
Rollout
Same host clone as #3754 — the timer runs the script directly from
~/Code/omni_home/omnibase_infra, which sits in detached HEAD, so a fast-forward pull will not move it. Not taken here: advancing that clone makes the next hourly tick reap several hundred images, which is an operator decision. The dry-run path (disk-gc.shwith no--execute) prints the full removal plan and is the right first step in that window.Evidence-Ticket: OMN-16367
Evidence-Source: OCC#10203