diff --git a/Fuchs/Docs/Decisions/0022-mfr-sync-transaction-blocking-accepted-risk.md b/Fuchs/Docs/Decisions/0022-mfr-sync-transaction-blocking-accepted-risk.md new file mode 100644 index 0000000..31772a1 --- /dev/null +++ b/Fuchs/Docs/Decisions/0022-mfr-sync-transaction-blocking-accepted-risk.md @@ -0,0 +1,65 @@ +--- +status: Accepted +date: 2026-09-16 +applyTo: + - "Fuchs_Database/dbo/Stored Procedures/mfr__updt__*.sql" + - "Fuchs_DataService/FdsMfrClient.cs" +supersededBy: "" +--- + +# 0022 — MFR sync transaction blocking during full resets is an accepted risk + +## Context +Every `mfr__updt__*` stored procedure (25 of them, one per synced mfr entity table) wraps its +DELETE-stale-references + MERGE-upsert logic in a single explicit `BEGIN TRANSACTION` / +`COMMIT TRANSACTION`, with `SET XACT_ABORT ON` and a `TRY/CATCH` that rolls back and logs to +`fds__admin_dberrors` on failure. This gives all-or-nothing atomicity for one entity's sync +batch: either every stale side-table row is cleaned up and the target table is upserted, or +none of it is, so a mid-batch failure can never leave orphaned side-table rows alongside a +stale/missing target row. + +`FdsMfrClient.cs`'s `CommandAfter` (which invokes the `mfr__updt__*` proc) and `CommandBefore`/ +staging `SqlBulkCopy` both use a 600s command timeout (`MergeCommandTimeoutSeconds`), raised +from `SqlCommand`'s 30s default specifically so a **full reset** of a large entity table (not +the normal incremental sync, which only carries the rows that changed since the last run) has +room to complete. Before that timeout was raised, a slow full-reset batch would self-abort at +30s — the client cancellation would cancel the batch and roll back the open transaction — +capping how long the transaction (and the locks it holds on the target table and the affected +side tables) could stay open. With the timeout at 600s, a genuinely large full-reset batch can +now hold that same transaction, and therefore those same locks, for up to ten minutes, during +which other readers of the live `mfr__*` tables (e.g. intranet pages querying companies, +contacts, invoices, …) can block waiting for the write transaction to release its locks — +instead of blocking, at most, on one DELETE or one MERGE statement individually. + +## Decision +Accept this as a known, documented risk rather than changing behavior. Full resets are rare +(a manual/administrative operation, not the routine incremental sync path) and are expected to +run outside of business hours. The atomicity the transaction provides — no orphaned side-table +rows left behind by a failed sync — is worth more than shortening the worst-case blocking +window for an operation this infrequent. + +## Consequences +- A full reset of a large `mfr__*` entity table can block ordinary intranet reads of that + table (and the side tables `mfr__*PartnerSet`, `mfr__#customvalues`, `mfr__#locations`, + `mfr__comments`) for as long as that reset's DELETE+MERGE takes to execute, up to the 600s + command timeout. +- Anyone scheduling or manually triggering a full reset of a large entity table should do so + during low-traffic hours, same as any other maintenance operation with table-level locking. +- If full resets become frequent enough, or large enough, for this blocking window to matter in + practice, revisit this decision — options considered and rejected here (see below) remain + available. + +## Alternatives considered +- **Drop the explicit transaction**, letting each DELETE/MERGE statement auto-commit + individually (the behavior before the transaction wrapping was added). Shortens the lock + window back to per-statement, but reintroduces the exact orphaned-data risk on mid-batch + failure that the transaction wrapping was added to prevent. Rejected — full-batch atomicity + was a deliberate improvement and giving it up trades a rare, low-traffic-hours blocking issue + for a correctness regression on every sync. +- **Enable Read Committed Snapshot Isolation (RCSI)** (`ALTER DATABASE ... SET + READ_COMMITTED_SNAPSHOT ON`) so readers use row versioning instead of blocking on the writer's + locks, keeping the transaction's atomicity intact while removing reader blocking entirely. + This is the architecturally cleanest fix, but it is a database-wide isolation behavior change + (affects every table, every query, tempdb version-store load) that needs its own testing and + sign-off rather than being folded into this change. Left as the preferred follow-up if the + accepted risk above turns out not to be acceptable in practice. diff --git a/Fuchs/js/intranet/modules/fis.usr.js b/Fuchs/js/intranet/modules/fis.usr.js index 6b0b7f3..d138088 100644 --- a/Fuchs/js/intranet/modules/fis.usr.js +++ b/Fuchs/js/intranet/modules/fis.usr.js @@ -49,14 +49,18 @@ let $usr = { }, /* Re-fetches a single user and swaps its grid row in place, keeping selection + ribbon (used after changes that only affect that one row: email, group membership). Falls back - to a full reload when the row is no longer in the grid. */ + to a full reload when the row is no longer in the grid. Looks up the current row node by + id inside the response handler (not before the request) so two overlapping calls — e.g. + two quick checkbox toggles in groupsDlg — each replace whatever row is live at the time + their own response arrives, instead of the second call operating on a node the first + already detached via replaceWith. */ refreshRow: function (rw) { - let old = $('#usr' + rw.useraccount_id); - if (old.length === 0) { $usr.renderLst(); return; } + if ($('#usr' + rw.useraccount_id).length === 0) { $usr.renderLst(); return; } $ocms.postXT({ url: $ocms.url('usr/list'), data: { useraccount_id: rw.useraccount_id }, success: function (r) { let fresh = (r.users || [])[0]; - if (!fresh) { $usr.renderLst(); return; } + let old = $('#usr' + rw.useraccount_id); + if (!fresh || old.length === 0) { $usr.renderLst(); return; } let selected = old.is('.selected'); let row = $usr.buildRow(fresh).toggleClass('selected', selected); old.replaceWith(row); diff --git a/Fuchs_DataService/FdsMfrClient.cs b/Fuchs_DataService/FdsMfrClient.cs index 5863547..899f183 100644 --- a/Fuchs_DataService/FdsMfrClient.cs +++ b/Fuchs_DataService/FdsMfrClient.cs @@ -16,7 +16,10 @@ public class FdsMfrClient : IDisposable private readonly MFRClient _mfrClient; private readonly ILogger _logger; - // Full-reset merges of large entity tables exceed SqlCommand's 30 s default. + // Full-reset merges of large entity tables exceed SqlCommand's 30 s default. Raising this + // also raises how long a full-reset's mfr__updt__* transaction can hold locks and block + // readers of the live mfr__* tables — an accepted, documented risk, see + // Docs/Decisions/0022-mfr-sync-transaction-blocking-accepted-risk.md. private const int MergeCommandTimeoutSeconds = 600; private static MFRClientCredentials DefaultCredentials() => @@ -259,7 +262,13 @@ public class FdsMfrClient : IDisposable try { tbl.AcceptChanges(); - var dtwa = new DatatableWriterAsync(tbl, FdsShared.FDSConnectionString()); + var dtwa = new DatatableWriterAsync(tbl, FdsShared.FDSConnectionString()) + { + // A large full-reset batch can exceed SqlBulkCopy's 30s default just as + // easily as the merge proc it feeds into — match CommandAfter's timeout + // so staging itself doesn't fail before the merge/fds__setStatus ever runs. + BulkCopyTimeoutSeconds = MergeCommandTimeoutSeconds + }; // One fixed debug copy per entity table (last failed batch) instead of a new t__ table per attempt. string fallbackTable = $"t_err_{tbl.TableName}"; dtwa.CommandAfterError = new SqlCommand( diff --git a/Fuchs_Database/FuchsDatabase.sqlproj b/Fuchs_Database/FuchsDatabase.sqlproj index 89fc8fe..7eb264c 100644 --- a/Fuchs_Database/FuchsDatabase.sqlproj +++ b/Fuchs_Database/FuchsDatabase.sqlproj @@ -146,8 +146,6 @@ - - diff --git a/Fuchs_Database/Security/fuchs_fds.login.sql b/Fuchs_Database/Security/fuchs_fds.login.sql deleted file mode 100644 index ed604f2..0000000 --- a/Fuchs_Database/Security/fuchs_fds.login.sql +++ /dev/null @@ -1,6 +0,0 @@ --- Server-level login referenced by the [fuchs_fds] database user. --- Exists only so the project model resolves (SQL71501); the real password --- is managed on the server and never taken from this script. -CREATE LOGIN [fuchs_fds] - WITH PASSWORD = N'placeholder-not-deployed'; - diff --git a/Fuchs_Database/Security/fuchs_web.login.sql b/Fuchs_Database/Security/fuchs_web.login.sql deleted file mode 100644 index 42cabfc..0000000 --- a/Fuchs_Database/Security/fuchs_web.login.sql +++ /dev/null @@ -1,6 +0,0 @@ --- Server-level login referenced by the [fuchs_web] database user. --- Exists only so the project model resolves (SQL71501); the real password --- is managed on the server and never taken from this script. -CREATE LOGIN [fuchs_web] - WITH PASSWORD = N'placeholder-not-deployed'; - diff --git a/Fuchs_Database/dbo/Stored Procedures/fis_admin_delUserGroup.sql b/Fuchs_Database/dbo/Stored Procedures/fis_admin_delUserGroup.sql index 3e40448..d3a6d8d 100644 --- a/Fuchs_Database/dbo/Stored Procedures/fis_admin_delUserGroup.sql +++ b/Fuchs_Database/dbo/Stored Procedures/fis_admin_delUserGroup.sql @@ -13,6 +13,11 @@ BEGIN SET NOCOUNT ON; DECLARE @utcnow datetime = GETUTCDATE(); DECLARE @auth smallint = [dbo].[fis_admin_getUserAuth](@authaccount); + -- Tracks whether THIS call actually soft-deleted an active group, so a never-existing or + -- already-deleted @usergroup_id reports failure instead of a false "ok:true" (the previous + -- version derived success from "not currently active", which is also true when nothing here + -- ever existed). + DECLARE @deleted bit = 0; IF @auth > 1 AND EXISTS (SELECT * FROM [dbo].[fis_usergroups] WHERE [usergroup_id] = @usergroup_id and [DateDeleted] is null) BEGIN @@ -21,10 +26,22 @@ BEGIN SET [UserDeleted] = @authaccount ,[DateDeleted] = @utcnow WHERE - [usergroup_id] = @usergroup_id; + [usergroup_id] = @usergroup_id + AND [DateDeleted] is null; + + IF @@ROWCOUNT > 0 SET @deleted = 1; + + -- Soft-delete the group's active memberships too, matching the confirm text + -- ("Die Mitgliedschaft aller Benutzer in dieser Gruppe geht dabei verloren."). + UPDATE [dbo].[fis_usergroups_members] + SET [UserDeleted] = @authaccount + ,[DateDeleted] = @utcnow + WHERE + [usergroup_id] = @usergroup_id + AND [DateDeleted] is null; END - SELECT CAST((CASE WHEN NOT EXISTS (SELECT * FROM [dbo].[fis_usergroups] WHERE [usergroup_id] = @usergroup_id and [DateDeleted] is null) THEN 1 ELSE 0 END) as bit); + SELECT @deleted; END \ No newline at end of file diff --git a/Fuchs_Database/dbo/Stored Procedures/fis_admin_setUserGroup.sql b/Fuchs_Database/dbo/Stored Procedures/fis_admin_setUserGroup.sql index 9c32074..6843fd6 100644 --- a/Fuchs_Database/dbo/Stored Procedures/fis_admin_setUserGroup.sql +++ b/Fuchs_Database/dbo/Stored Procedures/fis_admin_setUserGroup.sql @@ -17,9 +17,14 @@ BEGIN DECLARE @createdID as TABLE ([id] varchar(5)); - IF @auth > 1 + -- Renaming an existing group (@usergroup_id supplied) requires that id to currently be an + -- active group; otherwise the MERGE below would silently INSERT it as a brand-new group + -- (rename-of-unknown-id becoming create) or resurrect/UPDATE a soft-deleted row without + -- clearing its DateDeleted (leaving it deleted yet renamed). @usergroup_id = NULL (create) + -- always passes this check. + IF @auth > 1 AND (@usergroup_id IS NULL OR EXISTS (SELECT * FROM [dbo].[fis_usergroups] WHERE [usergroup_id] = @usergroup_id AND [DateDeleted] IS NULL)) BEGIN - + MERGE [dbo].[fis_usergroups] as TARGET USING (VALUES( ISNULL(@usergroup_id, [dbo].[fis_fn_id] ('fis_usergroups')) ,ENCRYPTBYKEY(@enc_key, @name) diff --git a/Fuchs_Database/dbo/Stored Procedures/mfr__updt__companies.sql b/Fuchs_Database/dbo/Stored Procedures/mfr__updt__companies.sql index cdbce8d..195dc50 100644 --- a/Fuchs_Database/dbo/Stored Procedures/mfr__updt__companies.sql +++ b/Fuchs_Database/dbo/Stored Procedures/mfr__updt__companies.sql @@ -20,6 +20,11 @@ BEGIN DECLARE @tmp_cmd nvarchar(1000) = N'SELECT DISTINCT * FROM ' + @tblname INSERT INTO @tmp EXECUTE [sp_executesql] @tmp_cmd; + -- Wrapping the DELETEs + MERGE below in one transaction gives all-or-nothing atomicity for + -- this entity's sync batch (same pattern in every mfr__updt__* proc). During a large full + -- reset this can hold locks on the affected tables for as long as the batch takes to run — + -- an accepted, documented risk, not an oversight. See + -- Docs/Decisions/0022-mfr-sync-transaction-blocking-accepted-risk.md before changing this. BEGIN TRANSACTION; IF @referencetable = 'mfr__companies'