Relax stop validation conditions

[goekay]

this behavior is coming from a real station in the field: the latest of meterValues is 1 second after stopTimestamp, because the station sampled stop timestamp first, and then later the timestamp for this meterValue entry.

a redacted version of the real station message can be found in #1992

[qodo-free-for-open-source-projects]

Review Summary by Qodo

Apply operational delta tolerance to stop timestamp validation

🐞 Bug fix 🧪 Tests

Walkthroughs

Description

• Apply operational delta tolerance to stop timestamp validation
  - Allows meter values up to 5 minutes after stop timestamp
  - Accounts for real-world clock drift and sampling delays
• Add test case for meter value 1 second after stop
• Refactor timestamp validation logic for consistency
  - Extract deltaMillis calculation for reuse
  - Improve code comments explaining tolerance rationale

Diagram

flowchart LR
  A["Meter Value Timestamp"] -->|"Check against Stop Timestamp"| B["Add Operational Delta Tolerance"]
  B -->|"Within tolerance"| C["Validation Passes"]
  B -->|"Exceeds tolerance"| D["Validation Fails"]
  E["Real-world Clock Drift"] -.->|"Justifies tolerance"| B

Loading

File Changes

1. src/main/java/de/rwth/idsg/steve/service/CentralSystemService16_ServiceValidator.java 🐞 Bug fix +10/-7

Add operational delta tolerance to stop timestamp check

• Apply operational delta tolerance to stop timestamp validation check
• Extract deltaMillis calculation for consistent reuse across timestamp validations
• Refactor stop timestamp check to use millisecond comparison with tolerance
• Improve code comments explaining clock drift and sampling delay tolerance

src/main/java/de/rwth/idsg/steve/service/CentralSystemService16_ServiceValidator.java

---

2. src/test/java/de/rwth/idsg/steve/service/CentralSystemService16ServiceValidatorTest.java 🧪 Tests +13/-1

Add test for meter value within stop tolerance

• Update existing test to use meter value 10 minutes after stop (exceeds 5-minute tolerance)
• Add new test case validating meter value 1 second after stop is allowed
• Add clarifying comment about operational delta tolerance threshold

src/test/java/de/rwth/idsg/steve/service/CentralSystemService16ServiceValidatorTest.java

---

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0) 📐 Spec deviations (0)

1. Missing delta boundary test 🐞 Bug ⚙ Maintainability

Description

The tests verify (a) well beyond the operational delta after stop and (b) one second after stop, but
do not assert that exactly stop.timestamp + operationalDelta is allowed, leaving an off-by-one
regression unguarded.

Code

src/test/java/de/rwth/idsg/steve/service/CentralSystemService16ServiceValidatorTest.java[R255-264]

+    @Test
+    public void validateStop_transactionDataOneSecondAfterStop_isAllowed() {
+        var tx = tx("100", DateTime.parse("2026-02-17T09:00:00Z"), null, null, null);
+
+        var params = stopParams(DateTime.parse("2026-02-17T10:00:00Z"), "200")
+            .withTransactionData(List.of(meterValue("2026-02-17T10:00:01Z")));
+        var result = validator.validateStop(tx, params);
+
+        Assertions.assertNull(result);
+    }

Evidence

The validator allows timestamps up to stopTimestamp + deltaMillis because it rejects only when
latest > stopTimestamp + deltaMillis. The test suite added coverage for +1s but has no coverage for
the exact boundary (+5m, given the default Duration.ofMinutes(5)), so a future operator change
(e.g., switching to >=) would not be caught.

src/main/java/de/rwth/idsg/steve/service/CentralSystemService16_ServiceValidator.java[153-166]
src/main/java/de/rwth/idsg/steve/service/CentralSystemService16_ServiceValidator.java[54-56]
src/test/java/de/rwth/idsg/steve/service/CentralSystemService16ServiceValidatorTest.java[210-220]
src/test/java/de/rwth/idsg/steve/service/CentralSystemService16ServiceValidatorTest.java[255-264]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The test suite does not cover the exact boundary condition `stopTimestamp + operationalDelta` (expected allowed). This leaves the key inequality (`>` vs `>=`) unprotected.
### Issue Context
`CentralSystemService16_ServiceValidator` uses a 5-minute default operational delta tolerance and rejects only when the latest meter value timestamp is *greater than* `stopTimestamp + delta`.
### Fix Focus Areas
- Add a test case asserting that a meter value at exactly `stopTimestamp + 5 minutes` is allowed.
- src/test/java/de/rwth/idsg/steve/service/CentralSystemService16ServiceValidatorTest.java[209-265]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Unsafe epoch millis addition 🐞 Bug ⛯ Reliability

Description

validateMeterValuesInternal computes tolerance windows via `toEpochMilli() +
deltaMillis/getMillis() + deltaMillis`, which is inconsistent with validateStart/validateStop and
can overflow silently for extreme timestamps.

Code

src/main/java/de/rwth/idsg/steve/service/CentralSystemService16_ServiceValidator.java[R151-159]

+        long deltaMillis = operationalDeltaForNow.toMillis();
+
+        if (latest.getMillis() > clock.instant().toEpochMilli() + deltaMillis) {
          return new SteveException("at least one MeterValue.timestamp is in the future");
      }
-        if (stopTimestamp != null && latest.isAfter(stopTimestamp)) {
-            return new SteveException("at least one MeterValue.timestamp is after stop.timestamp");
+        if (stopTimestamp != null) {
+            if (latest.getMillis() > stopTimestamp.getMillis() + deltaMillis) {
+                return new SteveException("at least one MeterValue.timestamp is after stop.timestamp");

Evidence

In the same validator class, validateStart and validateStop use
clock.instant().plus(operationalDeltaForNow).toEpochMilli(), but validateMeterValuesInternal now
uses manual addition with deltaMillis. This is a behavioral inconsistency within the same
component and introduces a silent-overflow hazard that Instant.plus(Duration) avoids by
construction.

src/main/java/de/rwth/idsg/steve/service/CentralSystemService16_ServiceValidator.java[58-69]
src/main/java/de/rwth/idsg/steve/service/CentralSystemService16_ServiceValidator.java[74-93]
src/main/java/de/rwth/idsg/steve/service/CentralSystemService16_ServiceValidator.java[148-166]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`validateMeterValuesInternal` manually adds `deltaMillis` to epoch millis for the tolerance checks. This is inconsistent with other methods in the same class and risks silent overflow.
### Issue Context
Other checks already use `clock.instant().plus(operationalDeltaForNow).toEpochMilli()`.
### Fix Focus Areas
- Replace `clock.instant().toEpochMilli() + deltaMillis` with `clock.instant().plus(operationalDeltaForNow).toEpochMilli()`.
- Replace `stopTimestamp.getMillis() + deltaMillis` with `stopTimestamp.plus(operationalDeltaForNow).getMillis()` (or equivalent safe arithmetic).
- src/main/java/de/rwth/idsg/steve/service/CentralSystemService16_ServiceValidator.java[148-166]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more

[juherr]

@goekay This is an interesting use case. I couldn’t find a clearly defined or recommended behavior for this scenario in the spec.

I have a couple of questions and thoughts regarding the current approach:

• What is the rationale behind allowing a 5-minute margin, but not more or less? Is this based on observed behavior from specific charge points or a general assumption?
• Would it make sense to make this margin configurable? For example, introducing a parameter that could be defined earlier in the process, potentially driven by charger model characteristics (with a sensible default like 5 minutes).
• Also, have you considered decoupling the start and stop margins? In practice, clock drift or buffering issues might impact them differently, so handling them independently could provide more flexibility.

Curious to hear your thoughts on this.

[goekay]

This is an interesting use case. I couldn’t find a clearly defined or recommended behavior for this scenario in the spec.

now... the validation/strictness suggestions opened a can of worms, and now we have a cat-and-mouse game with the stations ;). steve's behaviour will need multiple iterations, i fear, until becoming somewhat stable. we are experiencing the old "real-world vs theory (or spec)" conflict now. this is the reason i was hesitant to introduce these kinds of things so far in the life of steve. but also luckily with Powerfill, i have direct and immediate operational feedback and can adjust. this was not possible before Powerfill.

What is the rationale behind allowing a 5-minute margin, but not more or less? Is this based on observed behavior from specific charge points or a general assumption?

a somewhat general assumption, but also backed by observed behavior within our Powefill operations. most stations behave correctly. so, we are dealing with a small percentage of potential issues here. and there, 5 minute is generous enough while also being small enough to catch systematic issues.

Would it make sense to make this margin configurable? For example, introducing a parameter that could be defined earlier in the process, potentially driven by charger model characteristics (with a sensible default like 5 minutes).

dont want to right now. if more people want, sure, later but not right now.

Also, have you considered decoupling the start and stop margins? In practice, clock drift or buffering issues might impact them differently, so handling them independently could provide more flexibility.

i considered and discarded. see my previous answer.