[shunping]

Summary

The Trigger message in beam_runner_api.proto only says when each trigger is ready to fire. It doesn't say what happens when a trigger finishes, meaning it will never fire again. Runners depend on that: when a window's root trigger finishes, the window is closed, no more panes are emitted, and later elements are dropped.
Java and Python already behave this way, but the rules live only in SDK and runner code. Prism behaved differently until #40878. This PR adds the rules to the model as comments only; there are no schema changes.

A similar topic has been covered in Guard against "Trigger Finishing", a data loss risk by @kennknowles.

Changes

Trigger header comment

• Triggers are evaluated per key and window; each time the root trigger is ready, the current pane is output.
• When the root trigger finishes, the window is closed for that key:
  • the firing that finished it is the final pane (PaneInfo.is_last);
  • no further panes are produced, including at end of window or window expiry, regardless of ClosingBehavior / OnTimeBehavior;
  • elements arriving after the window closes are dropped, whether or not they are late relative to the watermark.
• A root trigger that may finish before the window expires can lose data, so SDKs call it "unsafe". Repeat makes a trigger never finish. Runners must still implement the closed-window rules for pipelines that use finishing triggers.
• A finishing subtrigger doesn't close the window on its own; it only affects its parent.

When each trigger type finishes

Trigger	Finishes	Java state machine
AfterAll	after firing once	AfterAllStateMachine#L111
AfterAny	after firing once	AfterFirstStateMachine#L108
AfterEach	when its last subtrigger finishes	AfterEachStateMachine#L161
AfterEndOfWindow	with no late_firings, on its firing after end of window; with late_firings, never before expiry (late trigger repeats implicitly)	AfterWatermarkStateMachine#L196-L204, #L258
AfterProcessingTime, AfterSynchronizedProcessingTime	after firing once	AfterDelayFromFirstElementStateMachine#L231
ElementCount	after firing once	AfterPaneStateMachine#L133
OrFinally	when finally fires, or main finishes, whichever comes first	OrFinallyStateMachine#L77-L104
Repeat	never; the subtrigger is reset when it finishes	RepeatedlyStateMachine#L89
Default, Always	never
Never	never before window expiry

ClosingBehavior.EMIT_ALWAYS: notes that it doesn't apply if the window was already closed by a finished root trigger. In that case no pane is emitted at expiry.

References

What "finishing" a root trigger means

• Java Trigger javadoc, Trigger.java#L41-L45:

  When the root trigger finishes (indicating it will never fire again), the window is closed and any new elements assigned to that window are discarded.

• Java runner, ReduceFnRunner.java:
  • L643-L655: elements for a closed window are dropped and counted in droppedDueToClosedWindow.
  • L853-L866 and L879-L883: end-of-window and expiry panes fire only while windowIsActiveAndOpen(), i.e. the window is active and its trigger hasn't finished. So a closed window gets no extra pane, whatever ClosingBehavior says.
• Python: once a window's trigger finishes, GeneralTriggerDriver in trigger.py marks it with a TOMBSTONE and skips later elements for it.
• Programming guide, §9.5 Composite triggers: triggers can emit "repeatedly, at most once", and orFinally makes a trigger "fire one final time and never fire again".

"Unsafe" triggers

• Java Trigger.mayFinish(), Trigger.java#L143-L147: "A top level trigger that finishes can cause data loss, so is rejected by GroupByKey validation."
• Java GroupByKey, GroupByKey.java#L185-L245: triggerIsSafe rejects root triggers that may finish, except Never, and end-of-window triggers with zero allowed lateness or with late firings. The error reads "Unsafe trigger '…' may lose data, did you mean to wrap it in Repeatedly.forever(...)?".
• Python:
  • trigger.py#L160-L181: DataLossReason.MAY_FINISH, "data that comes in after the trigger may be lost. Example: AfterCount(1) will stop firing after the first element."
  • trigger.py#L269-L294: may_lose_data().
  • core.py#L3524-L3540: GroupByKey raises "Unsafe trigger … may lose data" unless --allow_unsafe_triggers is set.
• Design doc: Guard against "Trigger Finishing", a data loss risk, linked from the Java error message.

[github-actions]

📋 Review Summary

This PR significantly improves the documentation of trigger messages in beam_runner_api.proto by explicitly specifying the finishing semantics and final pane behaviors for each trigger type, as well as the top-level window closing and garbage collection rules. The added comments are clear, thorough, and well-aligned with the Beam model and runner implementations.

🔍 General Feedback

• The distinction between trigger finishing (releasing buffered data and dropping subsequent elements) and window expiration (end of window + allowed lateness) is clearly articulated in the top-level Trigger message.
• A minor correction is suggested on Never to avoid conflating trigger-level firing with runner-level final pane emission at window garbage collection.

---

💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.

💬 Inline Findings

• model/pipeline/src/main/proto/org/apache/beam/model/pipeline/v1/beam_runner_api.proto (line 1244): 🟢 Low: The Never trigger itself never fires and never finishes (shouldFire is always false). When used as a root trigger, the output at end of window + allowed lateness is produced by the runner's window garbage collection mechanism (as described in the top-level Trigger comment) rather than Never itself firing or finishing. If Never is used as a subtrigger (e.g., inside AfterAll or AfterEach), it will never fire even at window GC.

Clarifying that Never itself never fires or finishes avoids ambiguity when it is composed inside other triggers.

  // Never fires or finishes. When used as the root trigger, a final pane is still emitted at end of window + allowed lateness based on ClosingBehavior.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.82%. Comparing base (0f1ac43) to head (d756c45).
⚠️ Report is 16 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #40879       +/-   ##
===========================================
+ Coverage        0   79.82%   +79.82%     
===========================================
  Files           0      494      +494     
  Lines           0    83573    +83573     
===========================================
+ Hits            0    66715    +66715     
- Misses          0    16858    +16858     

Flag	Coverage Δ
python	79.82% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[lostluck]

I would assume this means all the sub triggers have fired and finished, since they are all ready?

Finishes after firing all subtriggers once?

[kennknowles]

The root trigger should never finish. That's sort of the point of the doc linked.

[kennknowles]

The comments in the proto seems useful. However, we should not revive the idea of triggers "finishing", and we should remove it from any SDK it has snuck into. It is not very useful for saying "good enough, throw away the rest of the data" because triggers are not expressive enough for that - instead, use state/timers or some other filter. It is very easy to accidentally make a trigger that drops data that you don't realize. This happened a lot in the early days, hence I made the breaking change to remove it.

[kennknowles]

The usefulness of triggers finishing is moving from one trigger to another in a Sequence trigger. Which, TBH, is also not very useful.

[github-actions]

Assigning reviewers:

R: @chamikaramj added as fallback since no labels match configuration
Backup reviewer: damccorm (if the main reviewer is unresponsive, you can escalate to them by tagging their username)

This pull request likely touches a core component ("core" label). Please review with scrutiny.

Note: If you would like to opt out of this review, comment assign to next reviewer.

Available commands:

• stop reviewer notifications - opt out of the automated review tooling
• remind me after tests pass - tag the comment author after tests pass
• waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).