[TAM-2038] Fix alert schedule for sensors with aggregated values

[GonzoSpire]

No description provided.

[GonzoSpire]

@claude review it

[SoftFx]

@claude review it

[SoftFx]

@claude review it

[GonzoSpire]

@claude review it

[SoftFx]

@claude review it

[github-actions]

Claude finished @SoftFx's task in 1m 7s —— View job

---

PR Review: [TAM-2038] Fix alert schedule for sensors with aggregated values

• Gather context
• Read changed files
• Provide review feedback

---

The PR is a focused, consistent fix across 3 files. Here's the analysis:

---

BaseValue.cs

Change 1 — LastUpdateTime property (BaseValue.cs:92):

// Before
public DateTime LastUpdateTime => LastReceivingTime ?? ReceivingTime;
// After
public DateTime LastUpdateTime => LastReceivingTime ?? Time;

Correct fix. Time is the sensor's measurement timestamp, while ReceivingTime is the server-side ingestion time. For non-aggregated values, LastUpdateTime should fall back to Time (the data timestamp), not ReceivingTime.

Change 2 — TryAggregateValue (BaseValue.cs:116):

// Before
LastReceivingTime = value.ReceivingTime;
// After
LastReceivingTime = value.Time;

Consistent with Change 1. When aggregating values, the tracked "last update" time should use the sensor's Time, not the server-side ReceivingTime.

Minor naming concern: LastReceivingTime is now storing value.Time (sensor timestamp), but the field name implies server-side receive time. This naming inconsistency predates this PR, but it may be worth a future rename to avoid confusion (e.g., LastAggregatedTime). Not a blocker.

---

SensorPolicyCollection.cs

Both alert schedule checks changed from value.Time to value.LastUpdateTime (SensorPolicyCollection.cs:114 and SensorPolicyCollection.cs:186):

// Before
schedulePassed = _scheduleProvider.IsWorkingTime(policy.ScheduleId.Value, value.Time);
// After
schedulePassed = _scheduleProvider.IsWorkingTime(policy.ScheduleId.Value, value.LastUpdateTime);

This is the core bug fix. For aggregated sensors, value.Time is the timestamp of the first value in the aggregate, while value.LastUpdateTime (= LastReceivingTime ?? Time) reflects the most recent update. Using value.Time for schedule checking on aggregated values was incorrect — it could skip or incorrectly trigger alerts.

---

ApiCsvConverters.cs

// Before
value => value.LastReceivingTime is null ? nameof(BaseValue.ReceivingTime) : nameof(BaseValue.LastReceivingTime)
// After
value => value.LastReceivingTime is null ? nameof(BaseValue.Time) : nameof(BaseValue.LastReceivingTime)

Consistent with the BaseValue.cs changes — the CSV export column name now correctly reflects Time instead of ReceivingTime when there's no aggregated value.

---

Summary

The fix is correct and well-scoped. The three changes are consistent with each other and address a real bug where aggregated-value sensors were using the wrong timestamp for schedule evaluation. No issues blocking merge.

[github-actions]

PR Review: [TAM-2038] Fix alert schedule for sensors with aggregated values

Branch: TAM-2037 → master | Version bump: 3.40.19 → 3.40.20

---

Summary

This PR fixes alert schedule evaluation for aggregated sensor values by switching from server-side receive timestamps (ReceivingTime) to sensor-side measurement timestamps (Time) in three areas: the LastUpdateTime computed property, the aggregation accumulator, and schedule checks in policy collection. The intent is sound — schedule/TTL checks should reflect when the sensor measured the value, not when the server happened to receive it.

---

Issues Found

1. Semantic Inconsistency: LastReceivingTime Now Stores Measurement Time

File: HSMCommon/SensorValues/BaseValue.cs, line 116

// Before
LastReceivingTime = value.ReceivingTime;
// After
LastReceivingTime = value.Time;

LastReceivingTime is named to express when the server last received the value, but after this change it stores the sensor's measurement time (Time). Any code that reads LastReceivingTime expecting a server-side timestamp will now silently get sensor time instead. This is a hidden semantic contract violation that could cause subtle bugs in future code that relies on the field's name.

Suggestion: Consider renaming LastReceivingTime to LastAggregatedTime or LastMeasurementTime to reflect its new meaning, and update all usages accordingly. Alternatively, keep the old semantics and introduce a separate field.

---

2. Sensor Clock Skew Risk

File: HSMCommon/SensorValues/BaseValue.cs

With the old code, LastUpdateTime fell back to ReceivingTime — a server-side, authoritative UTC timestamp. With the new code it falls back to Time — a timestamp provided by the sensor client, which may:

• Be sourced from an unsynchronized or misconfigured clock.
• Arrive out of order (e.g. delayed delivery of a stale measurement).
• Be arbitrarily far in the past or future.

Schedule and TTL checks now depend on the sensor clock's accuracy. A sensor with a clock drifted significantly into the past could permanently suppress alerts (schedule never "passes"), or a future-dated sensor could trigger early timeouts.

Suggestion: Consider validating or clamping Time against ReceivingTime (e.g. reject values where Time is more than N minutes ahead of ReceivingTime), or at least document that schedule correctness depends on synchronized sensor clocks.

---

3. Potential Breaking Change in CSV Export Column Names

File: HSMServer/ApiObjectsConverters/ApiCsvConverters.cs, line 64

// Before
value => value.LastReceivingTime is null ? nameof(BaseValue.ReceivingTime) : nameof(BaseValue.LastReceivingTime)
// After
value => value.LastReceivingTime is null ? nameof(BaseValue.Time) : nameof(BaseValue.LastReceivingTime)

This lambda determines which property name appears as the column label in the "Last update time" CSV column. Changing it from "ReceivingTime" to "Time" is a user-visible change for non-aggregated sensors. Any downstream consumers (scripts, data pipelines, BI tools) parsing or mapping by that column name will silently receive a different field name without any version signal.

Suggestion: Verify that no downstream integrations depend on the previous column name "ReceivingTime" in this position, or include a changelog/migration note.

---

Positive Observations

• The fix correctly addresses the root cause: using value.LastUpdateTime in SensorPolicyCollection (both SensorTimeout and CalculateStorageResult) ensures that aggregated sensors use the most-recent-measurement time for schedule evaluation, rather than only the time of the first value in the aggregation window.
• The changes are minimal, focused, and touch exactly the right code paths.
• The version bump is appropriate for a bug fix release.

---

Verdict

The core logic fix is correct and the approach is reasonable. The main concerns are:

1. Medium: The LastReceivingTime field name is now misleading — this is a maintainability risk worth addressing with a rename.
2. Low–Medium: Schedule correctness now depends on sensor clock accuracy; document this assumption.
3. Low: CSV column name change is a quiet, potentially breaking behavioural change for external consumers.

None of these block the fix from being correct for its stated purpose, but the naming issue in particular is worth cleaning up to prevent future confusion.