Repository navigation
[Prism] Guard ClearOrderedListState against uninitialized state maps - #40626
jrmccluskey merged 2 commits into
Conversation
Problem:
In TentativeData.ClearOrderedListState (sdks/go/pkg/beam/runners/prism/internal/engine/data.go),
Prism indexed directly into d.state[stateID][w] and wrote the cleared slice back to
kMap[string(uKey)] without checking whether d.state, d.state[stateID], or winMap[w]
had been initialized:
winMap := d.state[stateID]
w := d.toWindow(wKey)
kMap := winMap[w]
data := kMap[string(uKey)]
...
kMap[string(uKey)] = StateData{Bag: cleared} // panic: assignment to entry in nil map
SDK harnesses (such as the Python SDK's SynchronousOrderedListRuntimeState.commit and
the Rust SDK's OrderedListCell) buffer ordered-list mutations in memory during a bundle
and flush pending StateRequest_Clear range requests before StateRequest_Append requests
at bundle commit. When a bundle clears an ordered-list state cell (or range) in a
window for which no prior bundle has persisted state on the runner, TentativeData.state
has no map for (stateID, window), kMap is nil, and Prism panics on line 336.
Fix:
Use the existing d.clearState(stateID, wKey) helper in ClearOrderedListState and return
early when kMap is nil, matching ClearBagState, ClearMultimapState, and
ClearMultimapKeysState.
Reproduction (Python SDK on PrismRunner):
```python
import apache_beam as beam
from apache_beam.coders import VarIntCoder
from apache_beam.options.pipeline_options import PipelineOptions
from apache_beam.testing.util import assert_that, equal_to
from apache_beam.transforms.userstate import OrderedListStateSpec
from apache_beam.utils.timestamp import Timestamp
class ClearOrderedListDoFn(beam.DoFn):
EVENTS = OrderedListStateSpec("events", VarIntCoder())
def process(self, element, events=beam.DoFn.StateParam(EVENTS)):
key, (ts, op) = element
if op == "CLEAR":
events.clear()
yield f"cleared:{key}"
else:
events.add((Timestamp(micros=ts), op))
entries = [f"{int(t.micros)}={v}" for t, v in events.read()]
yield f"entry:{key}:{','.join(entries)}"
if __name__ == "__main__":
options = PipelineOptions(["--runner=PrismRunner", "--environment_type=LOOPBACK"])
with beam.Pipeline(options=options) as p:
out = (
p
| beam.Create([("k", (30, 300)), ("k", (10, 100)), ("k", (0, "CLEAR"))])
| beam.ParDo(ClearOrderedListDoFn())
)
assert_that(
out,
equal_to(["entry:k:30=300", "entry:k:10=100,30=300", "cleared:k"]),
)
```
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40626 +/- ##
============================================
+ Coverage 56.16% 56.22% +0.05%
============================================
Files 1124 868 -256
Lines 178336 153512 -24824
Branches 1489 0 -1489
============================================
- Hits 100170 86305 -13865
+ Misses 75645 65367 -10278
+ Partials 2521 1840 -681
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Updated comments for clarity in data_test.go.
There was a problem hiding this comment.
📋 Review Summary
This PR fixes a nil map panic in Prism's TentativeData.ClearOrderedListState when clearing ordered list state for an uninitialized state ID or window. By using the existing d.clearState(stateID, wKey) helper and returning early when the key map is nil, the implementation aligns with ClearBagState, ClearMultimapState, and ClearMultimapKeysState.
🔍 General Feedback
- The fix is clean, idiomatic, and consistent with the rest of
TentativeData's state clearing methods. - Unit tests in
data_test.gothoroughly cover the uninitialized state map cases (d.state == nil, missingstateID, and missing window entry followed by an append).
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
|
Assigning reviewers: R: @jrmccluskey for label go. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
Problem:
In
TentativeData.ClearOrderedListState(sdks/go/pkg/beam/runners/prism/internal/engine/data.go), Prism indexed directly intod.state[stateID][w]and wrote the cleared slice back tokMap[string(uKey)]without checking whetherd.state,d.state[stateID], orwinMap[w]had been initialized:SDK harnesses (such as the Python SDK's
SynchronousOrderedListRuntimeState.commit) buffer ordered-list mutations in memory during a bundle and flush pendingStateRequest_Clearrange requests beforeStateRequest_Appendrequests at bundle commit. When a bundle clears an ordered-list state cell (or range) in a window for which no prior bundle has persisted state on the runner,TentativeData.statehas no map for(stateID, window),kMapis nil, and Prism panics on line 336.Fix:
Use the existing
d.clearState(stateID, wKey)helper inClearOrderedListStateand return early whenkMapis nil, matchingClearBagState,ClearMultimapState, andClearMultimapKeysState.Reproduction (Python SDK on PrismRunner):