[TAM-2039] Add affected sensors to table, add cached alert schedule result for bar sensors

[GonzoSpire]

No description provided.

[github-actions]

Code Review: TAM-2039 — Add affected sensors to table, add cached alert schedule result for bar sensors

Bugs / Logic Errors

1. Duplicate sensors in GetSensorsByAlertSchedule (TreeValuesCache.cs)

The continue inside the inner foreach only advances to the next policy, not the next sensor. If a sensor has multiple policies that share the same ScheduleId, it will be added to result more than once.

foreach (var policy in sensor.Policies)
{
    if (policy.ScheduleId == id)
    {
        result.Add(sensor);
        continue;   // ← skips remaining policies but does NOT exit the outer loop
    }
}

Fix: use break instead of continue, or restructure with Any():

if (sensor.Policies.Any(p => p.ScheduleId == id))
    result.Add(sensor);

---

2. Missing timezone conversion in AlertSchedule.IsWorkingTime(startTime, endTime) (AlertSchedule.cs)

The existing single-point overload converts the input UTC time to the schedule's configured timezone via ConvertUtcToLocalTime before checking working windows. The new interval overload skips this conversion entirely and compares raw startTime/endTime values directly against local working windows. This will produce incorrect results for any non-UTC timezone.

// Single-point version (correct)
public bool IsWorkingTime(DateTime dateTime)
{
    var localTime = ConvertUtcToLocalTime(dateTime);
    // ... checks against localTime

// Interval version (missing conversion)
public bool IsWorkingTime(DateTime startTime, DateTime endTime)
{
    var currentTime = startTime;   // ← no ConvertUtcToLocalTime call
    ...
}

---

3. IntervalCache grows unbounded (AlertScheduleProvider.cs)

CacheEntry.IntervalCache is keyed on exact (DateTime startTime, DateTime endTime) pairs. Bar sensors will produce a new unique pair on every data point — the cache will accumulate indefinitely and is only cleared when a schedule is saved/deleted (InvalidateCache). For high-frequency bar sensors this becomes a slow memory leak, and the caching provides no benefit since the same (open, close) pair rarely repeats.

Consider either:

• Dropping the per-interval cache entirely (computing is cheap), or
• Bounding the cache size (e.g. LRU or a fixed max entry count).

---

Code Quality

4. ArgumentException missing parameter name

Both AlertSchedule.IsWorkingTime and AlertScheduleProvider.IsWorkingTime throw:

throw new ArgumentException("Start time must be less than end time");

The conventional overload includes the parameter name for easier debugging:

throw new ArgumentException("Start time must be less than end time", nameof(startTime));

---

5. Commented-out code block in tree.js

A large block of old logic was left commented out rather than removed. Dead code should be deleted — git history preserves the original if it's needed later.

//if (jQuery.isEmptyObject(prevState) && prevState !== undefined) {
//    let jstreeState = JSON.parse(localStorage.getItem('jstree'));
//    ...
//}

---

6. Formatting issues

• AlertSchedulesController.cs — closing brace for the class body is misaligned (the } for GetAlertScheduleList sits at the same level as the namespace closing brace).
• Extra blank lines in _ProductList.cshtml and AlertScheduleProvider.cs.
• Both modified files (ITreeValuesCache.cs, AlertSchedulesController.cs, TreeValuesCache.cs) are now missing a trailing newline.

---

Minor / Positive Notes

• Renaming Milleseconds → MillisecondsInSecond and returning 0 (not roundtrip time) on failed ping is correct.
• Making TimeZoneList a static Lazy<> is a good performance improvement.
• InvalidateCache() correctly also clears IntervalCache.
• Improved error logging on schedule parse failure (catch (Exception ex)) is a nice observability win.
• The ConvertUtcToLocalTime fix for DateTimeKind.Local input is correct.

[github-actions]

Code Review — TAM-2039: Add affected sensors to table, cached alert schedule result for bar sensors

Overall this is a solid feature addition. There are two genuine bugs and a handful of design concerns worth addressing.

---

Bug 1 — Wrong 12-hour format in TimeSpan YAML serialization (AlertScheduleParser.cs)

emitter.Emit(new Scalar(timeSpan.ToString(@"hh\:mm")));

hh is the 12-hour clock format (00–11). Any window that starts or ends at 13:00–23:59 will be serialized incorrectly — e.g. 23:00 becomes "11:00". Use HH (24-hour):

emitter.Emit(new Scalar(timeSpan.ToString(@"HH\:mm")));

---

Bug 2 — Cleanup threshold removes still-active cache entries (AlertScheduleProvider.cs)

var cleanupThreshold = DateTime.UtcNow.AddMinutes(1);
if (kvp.Key.EndTime < cleanupThreshold)

UtcNow + 1 minute as the upper bound evicts intervals whose EndTime is up to one minute in the future, i.e. intervals that are currently being queried. The intent is clearly to evict past entries. Use DateTime.UtcNow (remove already-ended intervals) or DateTime.UtcNow.AddMinutes(-1) (add a small grace period):

var cleanupThreshold = DateTime.UtcNow;   // or .AddMinutes(-1) for grace period

---

Design concern — AlertScheduleProvider disposes a timer but never declares IDisposable

Dispose() is implemented but the class declaration does not include : IDisposable, and IAlertScheduleProvider does not extend IDisposable. ASP.NET Core's DI container will never call Dispose() unless the registration uses a disposable-aware lifetime or the interface declares it. The timer will leak until the process exits.

Either extend IAlertScheduleProvider : IDisposable or register the class directly as IDisposable in the DI container.

---

Design concern — Unbounded IntervalCache growth between cleanup runs

IntervalCache is keyed on (DateTime startTime, DateTime endTime). Every unique open/close pair for every bar sensor creates a new entry. With high-frequency bar data and a 5-minute cleanup cycle, this dictionary can grow very large. Consider bounding its size (e.g. a fixed-capacity LRU or a simple count cap with eager eviction) or confirm that bar sensor cardinality is low enough in practice.

---

Potential null-reference in GetSensorsByAlertSchedule (TreeValuesCache.cs)

if (sensor.Policies.TimeToLive.ScheduleId == id)

If TimeToLive is null for sensors that have no TTL policy, this throws a NullReferenceException. Add a null guard:

if (sensor.Policies.TimeToLive?.ScheduleId == id)

---

Minor — Commented-out block left in tree.js

The original if (jQuery.isEmptyObject(prevState) && prevState !== undefined) block is still present as a large comment. The new logic is correct (the old condition was always false when prevState was undefined). The dead comment block should be removed to keep the file readable.

---

Minor — fetch-depth: 1 in review workflow may break git diff

claude-review.yml was changed from fetch-depth: 0 to fetch-depth: 1. The review prompt asks Claude to run:

git diff origin/master...origin/TAM-2039

A shallow clone of depth 1 may not contain the merge-base commit, causing this command to fail with "no merge base". Either keep fetch-depth: 0 or switch to gh pr diff (which doesn't require local history).

---

Positive changes worth calling out

• PingResponse: Returning Value = 0 on ping failure instead of the (meaningless) roundtrip time is a correct fix.
• ConvertUtcToLocalTime: Handling DateTimeKind.Local inputs before passing to ConvertTimeFromUtc prevents a potential ArgumentException.
• TimeZoneList as a lazy static: A clear win — avoids re-enumerating all system time zones on every view-model construction.
• tree.js: The null-safety improvements (savedState check + try/catch) are good defensive additions.
• InvalidateCache(): Consolidating the cache-clearing logic into one method is cleaner than the previous two-line inline reset.

[github-actions]

Code Review: [TAM-2039] Add affected sensors to table, add cached alert schedule result for bar sensors

Overall this PR contains solid improvements: fixes a 12-hour vs 24-hour bug in schedule serialization, adds interval-based schedule evaluation for bar sensors, and shows affected sensors in the UI. A few issues worth addressing below.

---

Bugs / Logic Errors

1. AlertScheduleProvider.Dispose is not thread-safe

_disposed is read and written without synchronization. Two threads could both read _disposed = false simultaneously and double-dispose. Use Interlocked.Exchange or a lock:

// src/server/HSMServer.Core/AlertSchedule/AlertScheduleProvider.cs
public void Dispose()
{
    if (Interlocked.Exchange(ref _disposed, 1) == 0)
    {
        _cleanupTimer?.Dispose();
        GC.SuppressFinalize(this);
    }
}

private int _disposed = 0; // int, not bool, for Interlocked

2. IsWorkingTime(DateTime, DateTime) day-boundary edge case

In AlertSchedule.cs, when iterating days the dayStart for intermediate days (not the first) is set to midnight of that day, but dayEnd for non-final days is currentTime.Date.AddDays(1). This works correctly for intermediate days. However, when startTime and endTime are on different days, the first day's dayEnd is correctly midnight. The issue is that dayStart on the first iteration is currentTime (= converted startTime), which is fine, but subsequent iterations set currentTime = currentTime.Date.AddDays(1) — so dayStart becomes midnight of that day. This is the correct and intended behavior, but the range [midnight, midnight_next_day) misses verifying if the final day's window should use end rather than midnight. The if (date == end.Date) guard is only evaluated once per loop iteration — this logic is actually correct. No bug, just flag it as a subtle invariant worth a unit test covering a multi-day interval.

3. DateTimeKind.Unspecified treated silently as UTC

In AlertSchedule.ConvertUtcToLocalTime:

var utc = utcDateTime.Kind == DateTimeKind.Local
    ? utcDateTime.ToUniversalTime()
    : DateTime.SpecifyKind(utcDateTime, DateTimeKind.Utc); // Unspecified → treated as UTC

If BarBaseValue.OpenTime / CloseTime have Kind = Unspecified, they'll be silently treated as UTC. Consider logging a warning or asserting UTC kind in IsWorkingTime(Guid, DateTime, DateTime).

4. CI workflow: fetch-depth: 1 may break diff

# .github/workflows/claude-review.yml
- uses: actions/checkout@v4
  with:
    fetch-depth: 1  # was 0

Changing from full history to shallow depth of 1 means git diff origin/master...origin/<branch> will fail for PRs where the branches have diverged by more than one commit (the merge base won't be available). The review agent will get a fatal error on the diff command. Consider using fetch-depth: 0 or at minimum fetch-depth: 50.

---

Code Quality / Maintainability

5. Commented-out dead code in tree.js

The old if (jQuery.isEmptyObject(prevState) && prevState !== undefined) block is left as comments (lines 132–147). This clutters the file. Remove it entirely since the replacement code is already in place.

6. CleanupIntervalCache is public but should be private

// AlertScheduleProvider.cs
public void CleanupIntervalCache()  // only called by the Timer callback

This method is an internal implementation detail. It should be private (or at most internal for testing). Exposing it as public widens the surface area of the API unnecessarily.

7. CLEANUP_PERIOD naming convention

private readonly TimeSpan CLEANUP_PERIOD = TimeSpan.FromMinutes(5);

C# convention for private readonly fields is _camelCase. This should be _cleanupPeriod. ALL_CAPS is a Java/C convention not used in C#.

8. Indentation regression in AlertSchedulesController.cs

The closing brace of the AlertSchedulesController class lost its indentation:

-    }
+}
 }

The class closing brace should remain indented one level inside the namespace.

9. Extra blank lines in _ProductList.cshtml

Three consecutive blank lines were added (lines 238–240). One is sufficient.

---

Performance

10. GetSensorsByAlertSchedule is O(sensors) on every page load

// TreeValuesCache.cs
foreach (var sensor in _sensorsById.Values)  // iterates ALL sensors

This runs once per schedule when loading the AlertSchedules index page. For deployments with thousands of sensors and many schedules, this is O(sensors × schedules) per page request. Consider maintaining a reverse index (scheduleId → List<SensorId>) that is kept up-to-date in AddSensor / RemoveSensor and invalidated when a policy changes.

11. IntervalCache can grow quickly for high-frequency bar sensors

Each unique (OpenTime, CloseTime) pair creates a new cache entry. High-frequency bar sensors producing a new bar every second would generate 300 entries per 5-minute cleanup interval per sensor. For large deployments this could cause meaningful memory pressure. Consider bounding the cache size per entry (e.g., LRU with max capacity) or only caching if the interval is longer than some threshold.

---

Minor / Positive Notes

• Renaming Milleseconds → MillisecondsInSecond: good fix for the typo and improved clarity.
• Fixing hh → HH in WriteYaml: correct fix for 12h vs 24h schedule serialization bug.
• Implementing the previously throw new NotImplementedException() overload of WriteYaml: good.
• TimeZoneList made static Lazy<>: sensible optimization, timezone list doesn't change at runtime.
• Logging the exception in LoadSchedulesFromDatabase instead of silently swallowing it: good.
• IDisposable on AlertScheduleProvider: correct, necessary for the Timer.
• tree.js null-guard and JSON parse try/catch: both improvements over the original.