Skip to content

fix(geometry): limit NEW geometry images, grandfather the ones already stored - #2022

Draft
jcschaff wants to merge 3 commits into
masterfrom
fix/geometryspec-image-veto
Draft

fix(geometry): limit NEW geometry images, grandfather the ones already stored#2022
jcschaff wants to merge 3 commits into
masterfrom
fix/geometryspec-image-veto

Conversation

@jcschaff

Copy link
Copy Markdown
Member

Fixes #2021 — a 61,920,000 pixel image terminated two prod api pods with
OutOfMemoryError because GeometrySpec.vetoableChange detected it and only logged.

The obvious fix would have been worse than the bug

Uncommenting the throw looks like a one-line change. It isn't:

XmlReader:2006   new Geometry(version, newimage)
  -> GeometrySpec(Version, VCImage)
    -> setImage()
      -> vetoableChange()          <- the veto fires here
    -> catch (PropertyVetoException e) { throw new RuntimeException(...) }

The veto also fires when deserializing saved geometries. Restoring enforcement at the
historical IMAGE_SIZE_LIMIT of 4,000,000 would have made every stored geometry above that size
fail to open, with a RuntimeException.

And 4,000,000 is small: 159³ in 3D, or 2000×2000 in 2D. That is almost certainly why the line
was commented out in 2017 rather than fixed — enforcing it rejects real models.

What this does instead

The enforced ceiling is separated from the historical constant:

value behaviour
IMAGE_SIZE_LIMIT 4,000,000 unchanged, kept for callers — now only warns
IMAGE_SIZE_LIMIT_DEFAULT 50,000,000 enforced, overridable via vcell.geometry.imageSizeLimit

50,000,000 is deliberately just below the size observed to kill a JVM, not an ideal value.
That is the property that makes this safe: anything it rejects would have crashed the JVM anyway,
so no geometry that opens today stops opening — while the case that took out two pods is now
refused with a message naming the actual size, the limit, and the property to raise.

Refusing one geometry beats killing a pod that is serving everyone.

The number should be lowered once someone surveys the image sizes actually present in the database;
it is a property precisely so that does not need a code change.

Tests

GeometrySpecImageSizeTest, 4 cases. The important one is the middle:

@Test
public void imageOverHistoricalLimitButUnderCeilingIsStillAccepted() {
    // 200^3 = 8,000,000 -- twice the historical limit, well under the ceiling.
    // Enforcing the historical value here would break saved models.

That is the regression guard against the naive fix, not against the original bug.

The suite was verified to detect the defect, not merely to pass. With the 2017 behaviour
restored:

imageOverTheCeilingIsVetoed:62
  Expected java.beans.PropertyVetoException to be thrown, but nothing was thrown.
Tests run: 4, Failures: 1

The other three still passed, confirming they do not depend on the veto.

Wider run

vcell-core Fast: 579 run, 0 failures, 8 errors — all MathOverrideRoundTripTest (×7) and
VCellDataTest (×1), the environmental failures docs/BUILDING.md documents for a worktree
without the Poetry environments installed. One of them reports the poetry deprecation as its own
cause. Nothing geometry-related failed; VCImageTest passed all 3.

Not included

The limit value itself needs a real answer, and this PR does not pretend to have one. 50 M is
"safely below the thing we watched explode." Someone with database access should check what sizes
exist, then set the property accordingly.

Fixes #2021

🤖 Generated with Claude Code

https://claude.ai/code/session_018kr8SbzXtwW3gMVUgMfDDt

GeometrySpec.vetoableChange detected an oversized image and let it through -- the
throw was commented out in 1bf1b2a (2017-08-12), leaving only a warning in a
method whose sole purpose is to veto. On 2026-08-22 a 61,920,000 pixel image
(15.5x the limit) exhausted a 1000MB heap and terminated two prod api pods with
OutOfMemoryError seven seconds apart. See #2021.

The obvious fix -- uncomment the throw -- would have been worse than the bug.

This veto also fires when DESERIALIZING saved geometries: XmlReader:2006/2010 build
`new Geometry(version, image)`, which reaches setImage, and the GeometrySpec(Version,
VCImage) constructor converts a PropertyVetoException into a RuntimeException.
Restoring enforcement at the historical IMAGE_SIZE_LIMIT of 4,000,000 would therefore
have made every saved geometry above that size fail to OPEN. 4,000,000 is 159^3 in 3D
or 2000x2000 in 2D, which is small for modern imaging -- almost certainly why the line
was commented out in the first place rather than fixed.

So the limit is now separate from the historical constant:

  IMAGE_SIZE_LIMIT          4,000,000   unchanged, kept for callers, now only warns
  IMAGE_SIZE_LIMIT_DEFAULT 50,000,000   enforced ceiling, overridable via
                                        vcell.geometry.imageSizeLimit

50,000,000 is chosen conservatively: just BELOW the size observed to kill a JVM, not at
some ideal value. Anything it rejects would have crashed the JVM anyway, so no geometry
that opens today stops opening -- while the failure mode that took out two pods is now
refused with a message naming the size, the limit, and the property to raise. Refusing
one geometry beats killing a pod that is serving everyone. Lower it once someone has
surveyed the image sizes actually in the database.

Tests (GeometrySpecImageSizeTest, 4 cases) cover the veto, the configurability, and --
the one that matters -- that an image ABOVE the historical limit but BELOW the ceiling
is still accepted, which is what keeps existing models loadable.

Verified the test detects the defect rather than merely passing: with the 2017 behaviour
restored, imageOverTheCeilingIsVetoed fails with 'Expected PropertyVetoException to be
thrown, but nothing was thrown' while the other three still pass.

vcell-core Fast: 579 run, 0 failures, 8 errors -- all the MathOverrideRoundTripTest /
VCellDataTest environmental errors docs/BUILDING.md documents for a worktree without the
Poetry environments (one of them reports the poetry deprecation as its own cause).

Fixes #2021

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018kr8SbzXtwW3gMVUgMfDDt
…y stored

Replaces the load-time veto this branch previously carried. That veto was wrong in a
way only backward compatibility exposes: GeometrySpec.vetoableChange fires when
DESERIALIZING a stored geometry, and the constructor turns PropertyVetoException into
RuntimeException, so any enforced ceiling makes stored models above it impossible to
OPEN. The model from #2021 is 61,920,000 pixels and loads today -- the earlier 50 M
ceiling would have broken exactly the model it was meant to protect against.

So the load path only warns, and the limits apply where a NEW image is submitted:
ServerDocumentManager.saveGeometry / saveBioModel / saveMathModel, keyed on
image.getKey() == null. Anything already in the database is grandfathered forever.
That split is what lets the limit sit at a value the api can actually serve rather
than at the largest thing anyone ever stored.

Two limits, both properties:

  vcell.geometry.newImageSizeLimit     16,000,000 px
  vcell.geometry.newImageRegionLimit        2,000 regions

The second one is NOT the pixel-class limit that was discussed. A class limit was
implemented, measured, and abandoned, because the measurement says it is the wrong
quantity. On a 256^3 volume, one subvolume per concentric shell, regions only:

  pixel classes    2      4     16     32     64      128
  regions          2      4     16     32     64   14,050
  peak          124MB  196MB  244MB  209MB  300MB  1,652MB

Memory is FLAT in pixel-class count -- a 64-subvolume geometry is unremarkable at
300 MB. The jump at 128 is not the class count: at that resolution the shells fall
below one voxel thick and FRAGMENT, and it is the 14,050 resulting regions that cost
1.65 GB. A 16-class limit would have rejected the cheap 64-subvolume case and still
admitted a fragmented 2-class one; measured separately, a random 2-class image costs
2,111 MB at under 1 MP.

Region count is also the better test for "was this image ever segmented", since a
fragmented segmentation is exactly what an unsegmented image produces. It costs
nothing to check: the geometry already computed a RegionImage while being parsed.
RegionImage does already refuse >65535 regions, but only after doing the work, and
65535 is far above where memory turns.

The size limit is a statement about today's implementation, not about the science:
measured peak for one parse is 152 MB at 4.1 M px, ~450 MB at 16.8 M, 1,431 MB at
62.1 M, against a 1000 MB prod api heap that serves everything else too. Raise it
once the RegionImage memory work in #2023 lands.

checkNewImageAcceptable returns a reason string rather than throwing, so the server
save path, the REST layer and the desktop client can all use it without agreeing on
an exception type. An uncomputed region count (-1) skips the region check rather than
guessing.

Tests (11) pin both directions: that an image over the submission limit still LOADS
and that 64 pixel classes still load -- the backward-compatibility guarantees -- as
well as the refusals. manySubvolumesAreNotTreatedAsFragmentation is the one that
would have failed under the abandoned class limit.

vcell-core Fast: 586 run, 0 failures, 8 errors -- the MathOverrideRoundTripTest and
VCellDataTest errors docs/BUILDING.md documents for a worktree without the Poetry
environments.

Fixes #2021

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018kr8SbzXtwW3gMVUgMfDDt
@jcschaff

Copy link
Copy Markdown
Member Author

Reworked after Jim's direction that existing geometries must keep loading and only new submissions should be limited. Force of that point: the 50 M ceiling in the first commit would have broken the very model from #2021 — BioModel 101963252 is 61,920,000 px and loads today. vetoableChange fires on deserialization, so any enforced ceiling there makes stored models unopenable.

Now: load path warns only; limits are enforced at ServerDocumentManager.saveGeometry / saveBioModel / saveMathModel, keyed on image.getKey() == null. Anything already in the database is grandfathered forever — which is what lets the limit sit at a value the api can actually serve.

The second limit is on regions, not pixel classes. I implemented the 16-class limit as discussed, then measured it and abandoned it. 256³ volume, one subvolume per concentric shell, regions only:

pixel classes 2 4 16 32 64 128
regions 2 4 16 32 64 14,050
peak 124MB 196MB 244MB 209MB 300MB 1,652MB

Memory is flat in pixel-class count — 64 subvolumes is unremarkable. The jump at 128 isn't the class count: those shells fall below one voxel thick and fragment, and it's the 14,050 regions that cost 1.65 GB. A 16-class limit would have rejected the cheap 64-subvolume case and still admitted a fragmented 2-class one — measured separately, a random 2-class image costs 2,111 MB at under 1 MP.

Region count is also the better test for "was this ever segmented", and it's free: the geometry already computed a RegionImage while parsing.

Also confirms your point that N binary passes wouldn't blow up — VCell's labelling already doesn't scale with class count.

Defaults: vcell.geometry.newImageSizeLimit 16,000,000 px, vcell.geometry.newImageRegionLimit 2,000. The size figure is about today's implementation (152 MB at 4.1 M px → 1,431 MB at 62.1 M, vs a 1000 MB heap), not about the science — raise it once the memory work lands.

@jcschaff jcschaff changed the title fix(geometry): enforce the image-size veto instead of logging past it fix(geometry): limit NEW geometry images, grandfather the ones already stored Aug 22, 2026
@jcschaff
jcschaff marked this pull request as draft August 23, 2026 21:42
@jcschaff

Copy link
Copy Markdown
Member Author

Converted to draft. Superseded for now by the planning document in #2025, which records that this branch's approach was corrected twice: the original load-path veto would have made BioModel 101963252 — the model from the incident — impossible to open, and the pixel-class limit was measured to be the wrong quantity (memory is flat in class count; region count is the predictor). Both limits still in here are guesses calibrated against today's implementation. Decisions in #2025 first.

jcschaff added a commit that referenced this pull request Aug 24, 2026
#2026 (8a6e9f7) and #2027 (620ccd5) landed on master 2026-08-24. Admin-merged
because master requires a review and none was available, which bypasses the merge
queue, so regression.yml was triggered manually against master.

Everything else in this document is still open: #2022, #2023 and #2024 remain drafts
and none of the decisions in section 6 have been made.

Refs #2021, #2026, #2027

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018kr8SbzXtwW3gMVUgMfDDt
GeometrySpec.java is CRLF in master. An early text-mode edit on this branch rewrote
every line ending to LF, so the diff read 2988 changed lines for a ~90 line change --
unreviewable, and pure whitespace churn in a file with no styling standard.

Converted back in binary mode. The file was uniformly LF so the conversion is exact:
no ending was ambiguous, and the assertion that stripping CR reproduces the previous
bytes is in the script, so no content moved.

This is the same trap that later bit the pixel-retention work, where it was caught on
git diff --stat before pushing. Worth checking --stat after any scripted edit to this
tree; ServerDocumentManager.java is LF in master and was fine.

Refs #2021, #2025

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018kr8SbzXtwW3gMVUgMfDDt
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.

GeometrySpec image-size veto is commented out: a 62MP image OOM-killed two prod api pods

1 participant