From 3a62d86b771617d36215a11bfbfee153198f8f2b Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Tue, 26 May 2026 17:04:33 +0200 Subject: [PATCH] =?UTF-8?q?fix(backend/backup):=20PR2=20Codex=20findings?= =?UTF-8?q?=20=E2=80=94=20lease-loss=20=E2=86=92=20409=20+=20UI/contract?= =?UTF-8?q?=20alignment?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../RestoreTablePreflightValidatorTests.cs | 17 +++++++++++++++++ .../Functions/Backup/RestoreRowFunction.cs | 1 + .../Backup/CriticalTableRestoreService.cs | 14 ++++++++++++++ .../app/admin/backups/[backupId]/page.tsx | 5 ++++- 4 files changed, 36 insertions(+), 1 deletion(-) diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/RestoreTablePreflightValidatorTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/RestoreTablePreflightValidatorTests.cs index fa2e9db3..7cd28c79 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/RestoreTablePreflightValidatorTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/RestoreTablePreflightValidatorTests.cs @@ -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() { diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Backup/RestoreRowFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Backup/RestoreRowFunction.cs index 01afd7ea..16ff7169 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Backup/RestoreRowFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Backup/RestoreRowFunction.cs @@ -136,6 +136,7 @@ namespace AutopilotMonitor.Functions.Functions.Backup "RowChangedSinceValidation" => HttpStatusCode.Conflict, "CurrentRowChanged" => HttpStatusCode.Conflict, "MaintenanceInProgress" => HttpStatusCode.Conflict, + "MaintenanceLeaseLost" => HttpStatusCode.Conflict, _ => HttpStatusCode.BadRequest, }; diff --git a/src/Backend/AutopilotMonitor.Functions/Services/Backup/CriticalTableRestoreService.cs b/src/Backend/AutopilotMonitor.Functions/Services/Backup/CriticalTableRestoreService.cs index 111b2f12..196558ad 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/Backup/CriticalTableRestoreService.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/Backup/CriticalTableRestoreService.cs @@ -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. diff --git a/src/Web/autopilot-monitor-web/app/admin/backups/[backupId]/page.tsx b/src/Web/autopilot-monitor-web/app/admin/backups/[backupId]/page.tsx index 84f058f8..1363beeb 100644 --- a/src/Web/autopilot-monitor-web/app/admin/backups/[backupId]/page.tsx +++ b/src/Web/autopilot-monitor-web/app/admin/backups/[backupId]/page.tsx @@ -263,7 +263,10 @@ function RestoreInputRow({