fix(backend/backup): PR2 Codex findings — lease-loss → 409 + UI/contract alignment

Two findings from the post-commit Codex review of PR2 (single-row restore):

Medium — CriticalTableRestoreService.CommitRowAsync now maps the
MaintenanceLeaseHolder's renewal-loss path to BackupTerminalException
"MaintenanceLeaseLost" (HTTP 409 Conflict), instead of letting the
storage SDK's OperationCanceledException bubble up to the HTTP layer's
generic catch as a 500 InternalError. The catch condition gates on
!ct.IsCancellationRequested so a real client cancel still re-throws.

Low — RestoreInputRow (admin/backups/[backupId]/page.tsx) used to
disable the Preview button on empty pk/rk. The backend contract
(RestoreTablePreflightValidator) explicitly allows empty-string keys
(Azure Tables permits them) and only rejects null. UI now mirrors that.
Added a regression test pinning the empty-keys-allowed contract.

1980/1980 backend tests + 390/390 web tests green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Oliver Kieselbach
2026-05-26 17:04:33 +02:00
parent 9e689dc9d8
commit 3a62d86b77
4 changed files with 36 additions and 1 deletions
@@ -102,6 +102,23 @@ public class RestoreTablePreflightValidatorTests
Assert.Equal("InvalidKeys", ex.Code);
}
[Fact]
public void ValidateRowRequest_accepts_empty_partition_and_row_keys()
{
// Azure Tables permits empty-string PK/RK; the contract explicitly says
// "may be empty string but not null". Lock this so a future tightening of
// the validator doesn't silently break legal Azure Tables keys (UI in
// [backupId]/page.tsx mirrors this — Codex finding 2).
var req = new RestoreRowRequest
{
TableName = Constants.TableNames.AnalyzeRules,
PartitionKey = "",
RowKey = "",
Mode = RestoreRowMode.Preview,
};
Sut.ValidateRowRequest("backup1", req); // no throw
}
[Fact]
public void ValidateRowRequest_commit_requires_ifSha256()
{
@@ -136,6 +136,7 @@ namespace AutopilotMonitor.Functions.Functions.Backup
"RowChangedSinceValidation" => HttpStatusCode.Conflict,
"CurrentRowChanged" => HttpStatusCode.Conflict,
"MaintenanceInProgress" => HttpStatusCode.Conflict,
"MaintenanceLeaseLost" => HttpStatusCode.Conflict,
_ => HttpStatusCode.BadRequest,
};
@@ -255,6 +255,20 @@ namespace AutopilotMonitor.Functions.Services.Backup
};
}
}
catch (OperationCanceledException ex) when (!ct.IsCancellationRequested)
{
// The renewal loop cancelled handlerCts after a failed RenewAsync — the
// external client did NOT go away (ct still alive), so this is a server-side
// lease loss, not a client abort. Map to a stable 409 so the UI can prompt
// the operator to retry (the row-level write may not even have started).
// Without this guard the storage SDK's OperationCanceledException bubbles
// up to the HTTP layer's generic catch and renders as a 500.
var reason = holder.RenewalFailureReason ?? "handler cancelled without explicit renewal failure";
throw new BackupTerminalException(
"MaintenanceLeaseLost",
$"maintenance lease lost during commit — retry shortly ({reason})",
ex);
}
finally
{
// DisposeAsync stops the renewal loop deterministically before releasing.
@@ -263,7 +263,10 @@ function RestoreInputRow({
<button
type="button"
onClick={() => onSubmit(tableName, pk, rk)}
disabled={disabled || !tableName || pk === "" || rk === ""}
// Backend contract (RestoreTablePreflightValidator) allows empty PK/RK
// — only null is rejected. Match that here so an Azure Table row with
// a literal empty key (legal per spec) can be previewed/restored.
disabled={disabled || !tableName}
className="px-3 py-1.5 bg-blue-600 hover:bg-blue-700 disabled:opacity-50 text-white text-sm rounded-md transition-colors"
>
Preview restore