WellConnections: bound the Cartesian->compressed lookup

[hnil]

Narrowed after review — this is now only the out-of-bounds read. The LGR-specific handling and its test have been dropped; see the discussion below.

What

WellConnections::init translates each connection into a compressed cell index:

cart_grid_idx = i + nx*(j + ny*k);
compressed_idx = cartesian_to_compressed[cart_grid_idx];

and does the same for possibleFutureConnections. Neither lookup checks that the computed position is inside cartesian_to_compressed before indexing it.

The map is sized by the level-zero Cartesian grid, and a connection is not obliged to carry a position in that grid. When the position lands past the end, that is an out-of-bounds read during load balancing.

The change

Both lookups go through a helper that returns −1 for a position outside the map. −1 then flows into the existing "ignore connections in inactive cells" test — no new control flow, and no new notion of a skipped connection: out-of-map is handled exactly as inactive already is.

The bound is cartesian_to_compressed.size(), i.e. the container actually being indexed, rather than the product of cartesianSize. The two agree in the CpGrid constructor, but the four-argument constructor takes them independently, so bounding against the container is the honest check.

Nothing changes for a position that was already in range, so grids whose connections all address level zero are unaffected.

On the review comments

it is 10 steps ahead of our schedule and we should first make the simple cases work

Understood, and dropped accordingly. The previous version also skipped connections by refinement level, which is a modelling decision about what belongs in the level-zero well graph. That is the part that was ahead of schedule, and it is gone — nothing here depends on refinement any more.

Some of this code is more dangerous than before

Fair on the earlier version: it introduced a second, silent reason for a connection to disappear from the well graph, including for ordinary level-zero connections, with no diagnostic. This version does not add a skip path at all — it reuses the inactive-cell one — so the only behavioural difference is that a read which was undefined now returns −1.

Testing

test_graphofgrid and the zoltan/partition tests pass. Built against opm-common master.

No unit test is included: the defect is an out-of-bounds read, and the value it returns is unspecified, so an assertion on the result is not decisive. It is visible under a sanitizer build. Happy to add an ASan-guarded case if you would like one.

Not needed by anything in flight

For scheduling: none of the opm-common or opm-simulators PRs currently open depend on this. Pending it costs nothing on my side — say the word and I will close it and carry the fix locally.

[blattms]

Thanks a lot. While this is generally appreciated it is 10 steps ahead of our schedule and we should first make the simple cases work.

Some of this code is more dangerous than before. Probably a symptom of coding agents just somehow making things work ...

[hnil]

Context for reviewers: this is part of a group of independent LGR fixes, none of which depend on each other to build or merge.

Closest relatives:

• RegionCache: skip LGR-completed connections when building the global-grid region cache opm-common#5251 — the same class of bug in RegionCache::buildCache: an LGR-local index read as a global one. Same reasoning about why the in-range case is the dangerous one.
• LGR: rebuild Cartesian->compressed map after refinement, enable serial summary output, fix rank-asymmetric throw opm-simulators#7245 — LGR well/rank handling on the simulator side, including a rank-asymmetric throw at np >= 6. That PR notes a remaining gap where a well's assigned rank and its refinement box's refining rank can diverge; this PR is a prerequisite for reasoning about that cleanly, since it stops LGR-local positions from anchoring wells to unrelated coarse cells during load balancing.

[hnil]

On more contest is: we have a prototype LGR implementation which can produce results for some case. This make it possible to fix a lot of output issues and infrastructure problems.. The idea is that fixing all this outputs now will be usefull both for getting the new code work and be able to use the prototype. The idea for the opm-grid requests was to make the changes need in opm-common and opm-simulators possible, but also some of the bugs found.

[hnil]

Thanks — both points taken, and I've narrowed this to just the out-of-bounds read.

The refinement-level handling is gone. You're right that it was a modelling decision about what belongs in the level-zero well graph, and that isn't this PR's business. It also introduced a second, silent way for a connection to vanish from the graph — including ordinary level-zero ones — which is a fair reading of "more dangerous". What's left adds no skip path at all: an out-of-map position returns -1 and flows into the existing inactive-cell test, so the only behavioural change is that a read which was undefined now isn't.

I also moved the bound from the product of cartesianSize to cartesian_to_compressed.size() — the container actually being indexed. Those agree in the CpGrid constructor but not necessarily in the four-argument one.

On scheduling: nothing I have open in opm-common or opm-simulators depends on this, so there's no pressure from my side. Happy to park it if you'd rather not touch this path yet — just say so and I'll close it and carry the fix locally until the simple cases are where you want them.

[hnil]

What should I do with this? @blattms. It fix a bug and run all tests.