-
-
Notifications
You must be signed in to change notification settings - Fork 82
RIC-T39 Backend IC work #417
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| using System; | ||
|
|
||
| namespace Resgrid.Model | ||
| { | ||
| /// <summary> | ||
| /// Entities that carry a <see cref="ModifiedOn"/> change cursor. It is stamped on every insert and update | ||
| /// and drives two offline-first concerns: the delta-sync "changed since" query (pull) and last-write-wins | ||
| /// conflict resolution on reconnect. See <c>docs/architecture/offline-first-architecture.md</c>. | ||
| /// </summary> | ||
| public interface IChangeTracked | ||
| { | ||
| DateTime? ModifiedOn { get; set; } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| using System.Collections.Generic; | ||
|
|
||
| namespace Resgrid.Model | ||
| { | ||
| /// <summary> | ||
| /// Delta payload for offline sync: every change-tracked incident-command row whose <see cref="IChangeTracked.ModifiedOn"/> | ||
| /// (or, for the append-only timeline, OccurredOn) is newer than the client's last sync cursor, scoped to a department. | ||
| /// Soft-deleted / closed / released rows ARE included (with their state columns set) so the client can remove or | ||
| /// update them locally. The client stores <see cref="ServerTimestampMs"/> and passes it back as the next `since`. | ||
| /// Ad-hoc resources are not change-tracked and are pulled separately (full refetch). See | ||
| /// docs/architecture/offline-first-architecture.md. | ||
| /// </summary> | ||
| public class IncidentCommandChanges | ||
| { | ||
| /// <summary>Server clock (Unix epoch ms) captured at the start of the read; the client's next-sync cursor.</summary> | ||
| public long ServerTimestampMs { get; set; } | ||
|
|
||
| public List<IncidentCommand> Commands { get; set; } = new List<IncidentCommand>(); | ||
|
|
||
| public List<CommandStructureNode> Nodes { get; set; } = new List<CommandStructureNode>(); | ||
|
|
||
| public List<ResourceAssignment> Assignments { get; set; } = new List<ResourceAssignment>(); | ||
|
|
||
| public List<TacticalObjective> Objectives { get; set; } = new List<TacticalObjective>(); | ||
|
|
||
| public List<IncidentTimer> Timers { get; set; } = new List<IncidentTimer>(); | ||
|
|
||
| public List<IncidentMapAnnotation> Annotations { get; set; } = new List<IncidentMapAnnotation>(); | ||
|
|
||
| public List<IncidentRoleAssignment> Roles { get; set; } = new List<IncidentRoleAssignment>(); | ||
|
|
||
| public List<IncidentAdHocUnit> AdHocUnits { get; set; } = new List<IncidentAdHocUnit>(); | ||
|
|
||
| public List<IncidentAdHocPersonnel> AdHocPersonnel { get; set; } = new List<IncidentAdHocPersonnel>(); | ||
|
|
||
| public List<CommandLogEntry> TimelineEntries { get; set; } = new List<CommandLogEntry>(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -320,8 +320,36 @@ public async Task<List<ResolvedCheckInTimer>> ResolveAllTimersForCallAsync(Call | |
|
|
||
| public async Task<CheckInRecord> PerformCheckInAsync(CheckInRecord record, CancellationToken cancellationToken = default) | ||
| { | ||
| // Offline idempotency: when the client supplies its outbox event id, a replayed check-in returns the | ||
| // original record instead of inserting a duplicate. Dedup is in-memory over the call's (small) check-in | ||
| // set, so only replayed events — which carry a key — pay the extra read; live UI check-ins skip it. | ||
| if (!string.IsNullOrWhiteSpace(record.IdempotencyKey)) | ||
| { | ||
| var existing = await _recordRepository.GetByCallIdAsync(record.CallId); | ||
| var duplicate = existing?.FirstOrDefault(r => r.IdempotencyKey == record.IdempotencyKey); | ||
| if (duplicate != null) | ||
| return duplicate; | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| record.Timestamp = DateTime.UtcNow; | ||
| var saved = await _recordRepository.SaveOrUpdateAsync(record, cancellationToken); | ||
|
|
||
| CheckInRecord saved; | ||
| try | ||
| { | ||
| saved = await _recordRepository.SaveOrUpdateAsync(record, cancellationToken); | ||
| } | ||
| catch (Exception) when (!string.IsNullOrWhiteSpace(record.IdempotencyKey)) | ||
| { | ||
| // The in-memory pre-check above is check-then-insert and races under concurrent replays; the filtered | ||
| // unique index UX_CheckInRecords_Department_IdempotencyKey is the real guard. If we lost the race, | ||
| // adopt the winner — same idempotent result as the pre-check — rather than surfacing a 500 (which would | ||
| // just trigger another retry). | ||
| var existing = await _recordRepository.GetByCallIdAsync(record.CallId); | ||
| var winner = existing?.FirstOrDefault(r => r.IdempotencyKey == record.IdempotencyKey); | ||
| if (winner != null) | ||
| return winner; | ||
|
Comment on lines
+341
to
+350
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Verify which provider/repository exception types represent unique constraint violations
# so the catch filter can be narrowed without guessing.
rg -nP -C3 '(SaveOrUpdateAsync|SqlException|PostgresException|DbUpdateException|unique constraint|duplicate key|2601|2627|23505)' Core Providers -g '*.cs'Repository: Resgrid/Core Length of output: 50369 🏁 Script executed: python3 - <<'PY'
code_snippet = """
try
{
saved = await _recordRepository.SaveOrUpdateAsync(record, cancellationToken);
}
catch (Exception) when (!string.IsNullOrWhiteSpace(record.IdempotencyKey))
{
var existing = await _recordRepository.GetByCallIdAsync(record.CallId);
var winner = existing?.FirstOrDefault(r => r.IdempotencyKey == record.IdempotencyKey);
if (winner != null)
return winner;
throw;
}
"""
# Analyze the behavior of `throw` in this specific context
analysis = """
The snippet uses `catch (Exception)` without specifying a variable name.
Inside the block:
1. `var existing = await ...` is an I/O call. If this call fails, an *new* exception is awaited.
- If it throws an `OperationCanceledException`, that exception bubbles up immediately. The original `Exception` is effectively lost/swapped for the new failure if the await fails, or the new one propagates.
- If it succeeds but `winner` is null, the code hits `throw;`.
2. C# `throw;` with no arguments re-throws the *original* exception caught by the `catch` block (the one from `SaveOrUpdateAsync`).
3. Therefore, if `winner` is null, the original exception propagates correctly.
However, the risk remains if `SaveOrUpdateAsync` throws an exception that is NOT a unique violation (e.g., FK constraint violation "23503", Serialization error, or Timeout), and `existing` query *successfully* returns a record (perhaps from a different idempotency key or just a stale read).
If `winner` is found (even if it's the wrong idempotency key? No, the code checks `r.IdempotencyKey == record.IdempotencyKey`), then it returns the winner.
The logic `if (winner != null)` only returns if a record matches the key.
If the original exception was a Timeout, but `winner` happens to exist in the DB with the matching key (from a previous retry that partially completed), the current code treats it as a success.
CRITICAL ISSUE: The exception filter `when (!string.IsNullOrWhiteSpace(record.IdempotencyKey))` applies to the ENTRY of the catch block.
It does NOT guard against:
1. Non-unique exceptions (like Foreign Keys) where a matching record happens to exist.
2. It is safer to check the exception type explicitly.
"""
print(analysis)
PYRepository: Resgrid/Core Length of output: 1853 Limit retry adoption strictly to unique-key violations. The current retry logic catches all exceptions regardless of type, risking the masking of valid failures (e.g., timeouts, foreign key violations) as successful replays if a matching row happens to exist. For example, if a transaction times out but a partial write created the record, the code returns success even though the original operation failed with a non-unique-key error. Refine the exception handling to filter only for PostgreSQL unique constraint violations ( Proposed Fixtry
{
saved = await _recordRepository.SaveOrUpdateAsync(record, cancellationToken);
}
catch (Npgsql.PostgresException ex) when (ex.SqlState == "23505" && !string.IsNullOrWhiteSpace(record.IdempotencyKey))
{
var existing = await _recordRepository.GetByCallIdAsync(record.CallId, cancellationToken);
var winner = existing?.FirstOrDefault(r => r.IdempotencyKey == record.IdempotencyKey);
if (winner != null)
return winner;
throw;
}🤖 Prompt for AI Agents |
||
| throw; | ||
| } | ||
|
|
||
| // Real-time board refresh is best-effort: the check-in is already persisted, so a CQRS/Redis | ||
| // publish failure must not fail the check-in — that would 500 the caller and a retry would | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Doc comment contradicts the model and the actual delta behavior.
The remark says ad-hoc resources "are not change-tracked and are pulled separately (full refetch)", yet this payload exposes
AdHocUnits/AdHocPersonnel(Lines 32-34) andGetAdHocChangesSinceAsyncfilters them byModifiedOn(delta, not full refetch). This will mislead clients implementing the sync contract.📝 Suggested doc fix
📝 Committable suggestion
🤖 Prompt for AI Agents