From a10c5d6ffcd3cebf0f1326853a942c7bcaf56e7e Mon Sep 17 00:00:00 2001 From: JSv4 Date: Fri, 14 Aug 2026 07:03:01 -0500 Subject: [PATCH 1/5] Add in-session mutation transaction replay --- Docxodus/DocxSession.cs | 9 + npm/src/types.ts | 3 + python/src/docx_scalpel/enums.py | 3 + tools/mcp-server/Dispatcher.cs | 208 +++++++++++-- tools/mcp-server/MutationTransactions.cs | 364 +++++++++++++++++++++++ tools/mcp-server/SessionStore.cs | 96 +++++- tools/mcp-server/ToolCatalog.cs | 5 +- 7 files changed, 646 insertions(+), 42 deletions(-) create mode 100644 tools/mcp-server/MutationTransactions.cs diff --git a/Docxodus/DocxSession.cs b/Docxodus/DocxSession.cs index b3d3508c..bd3d7f47 100644 --- a/Docxodus/DocxSession.cs +++ b/Docxodus/DocxSession.cs @@ -1697,6 +1697,15 @@ public enum EditErrorCode /// A mutation batch step names an unsupported operation or a read-only action. InvalidBatchStep, + /// A transaction identity was supplied where it cannot safely identify an applying batch. + InvalidTransaction, + + /// A transaction id was already reserved for a different canonical request. + TransactionConflict, + + /// The exact response for a known transaction was evicted from bounded retention. + TransactionResultEvicted, + /// The revision family is visible but has no safe selective resolver. RevisionUnsupported, diff --git a/npm/src/types.ts b/npm/src/types.ts index 4b42efca..094d7e96 100644 --- a/npm/src/types.ts +++ b/npm/src/types.ts @@ -1354,6 +1354,9 @@ export type EditErrorCode = | "revision_not_found" | "precondition_failed" | "invalid_batch_step" + | "invalid_transaction" + | "transaction_conflict" + | "transaction_result_evicted" | "hyperlink_not_found" | "bookmark_not_found" | "duplicate_bookmark_name" diff --git a/python/src/docx_scalpel/enums.py b/python/src/docx_scalpel/enums.py index 662f4f06..39bf8adb 100644 --- a/python/src/docx_scalpel/enums.py +++ b/python/src/docx_scalpel/enums.py @@ -179,6 +179,9 @@ class EditErrorCode(str, Enum): EMPTY_COMMENT_SPAN = "empty_comment_span" REVISION_NOT_FOUND = "revision_not_found" INVALID_BATCH_STEP = "invalid_batch_step" + INVALID_TRANSACTION = "invalid_transaction" + TRANSACTION_CONFLICT = "transaction_conflict" + TRANSACTION_RESULT_EVICTED = "transaction_result_evicted" HYPERLINK_NOT_FOUND = "hyperlink_not_found" BOOKMARK_NOT_FOUND = "bookmark_not_found" DUPLICATE_BOOKMARK_NAME = "duplicate_bookmark_name" diff --git a/tools/mcp-server/Dispatcher.cs b/tools/mcp-server/Dispatcher.cs index ec7ba900..41fb6ed5 100644 --- a/tools/mcp-server/Dispatcher.cs +++ b/tools/mcp-server/Dispatcher.cs @@ -26,29 +26,48 @@ namespace Docxodus.McpServer; /// internal static class Dispatcher { - public static string Call(SessionStore store, string tool, JsonElement args) => tool switch - { - "docxodus_open" => Open(store, args), - "docxodus_save" => Save(store, args), - "docxodus_close" => Close(store, args), - "docxodus_get_content" => GetContent(store, args), - "docxodus_preview" => Preview(store, args), - "docxodus_pagination" => Pagination(store, args), - "docxodus_search" => Search(store, args), - "docxodus_edit" => Edit(store, args), - "docxodus_format" => Format(store, args), - "docxodus_create" => Create(store, args), - "docxodus_list" => ListTool(store, args), - "docxodus_comment" => Comment(store, args), - "docxodus_links" => Links(store, args), - "docxodus_images" => Images(store, args), - "docxodus_content_controls" => ContentControls(store, args), - "docxodus_annotate" => Annotate(store, args), - "docxodus_track_changes" => TrackChanges(store, args), - "docxodus_mutations" => Mutations(store, args), - "docxodus_table" => Table(store, args), - _ => throw new McpToolException($"unknown tool: {tool}"), - }; + public static string Call(SessionStore store, string tool, JsonElement args) + { + // Transaction identities belong only to an applying batch. Rejecting the property at + // this central seam also covers future direct tools instead of silently ignoring it. + if (tool != "docxodus_mutations" + && args.ValueKind == JsonValueKind.Object + && args.TryGetProperty("transactionId", out _)) + throw new McpToolException( + "transactionId is only valid on a mutating docxodus_mutations batch"); + + if (tool == "docxodus_open") return Open(store, args); + if (tool == "docxodus_close") return Close(store, args); + // Static capability discovery has no document state to serialize against. + if (tool == "docxodus_images" && OptStr(args, "action") == "capabilities") + return Images(store, args); + + // Session-bound calls are synchronous from lookup through serialized response creation. + // This includes reads and saves, because their relative order with a mutation/replay is + // observable, and makes HTTP's request-level concurrency safe without transport locks. + var sessionId = Str(args, "sessionId"); + return store.Dispatch(sessionId, () => tool switch + { + "docxodus_save" => Save(store, args), + "docxodus_get_content" => GetContent(store, args), + "docxodus_preview" => Preview(store, args), + "docxodus_pagination" => Pagination(store, args), + "docxodus_search" => Search(store, args), + "docxodus_edit" => Edit(store, args), + "docxodus_format" => Format(store, args), + "docxodus_create" => Create(store, args), + "docxodus_list" => ListTool(store, args), + "docxodus_comment" => Comment(store, args), + "docxodus_links" => Links(store, args), + "docxodus_images" => Images(store, args), + "docxodus_content_controls" => ContentControls(store, args), + "docxodus_annotate" => Annotate(store, args), + "docxodus_track_changes" => TrackChanges(store, args), + "docxodus_mutations" => Mutations(store, args), + "docxodus_table" => Table(store, args), + _ => throw new McpToolException($"unknown tool: {tool}"), + }); + } // ─── Lifecycle ────────────────────────────────────────────────────── @@ -829,6 +848,90 @@ private static string FilterRevisions(string revisionsJson, string? author, stri private static string Mutations(SessionStore store, JsonElement args) { var liveSession = Session(store, args); + var transactionId = TransactionId(args); + if (transactionId is null) + return ExecuteMutationRequest(liveSession, args, transactional: false); + + // Canonicalization also performs the duplicate-key rejection. It deliberately precedes + // preview policy validation so an ambiguous request can never acquire a transaction id. + var requestFingerprint = MutationTransactions.Fingerprint(args); + var identity = new MutationTransactionIdentity( + MutationTransactions.SchemaVersion, transactionId, requestFingerprint); + if (RequestsPreview(args)) + { + return MutationTransactions.SerializeFailure( + RequestedCoreMode(args), + preview: true, + SafeVersion(liveSession), + EditErrorCode.InvalidTransaction, + "transactionId is not valid for preview or dry-run mutation batches", + "transaction"); + } + + var decision = liveSession.MutationTransactions.Begin(transactionId, requestFingerprint); + switch (decision.Kind) + { + case MutationTransactionDecisionKind.Replay: + return decision.SerializedResponse!; + case MutationTransactionDecisionKind.Conflict: + { + var original = decision.ExistingIdentity?.RequestFingerprint ?? "unknown"; + var conflict = MutationTransactions.SerializeFailure( + RequestedCoreMode(args), + preview: false, + SafeVersion(liveSession), + EditErrorCode.TransactionConflict, + $"transactionId is already bound to a different request fingerprint ({original})", + "transaction"); + return MutationTransactions.AttachIdentity(conflict, identity); + } + case MutationTransactionDecisionKind.ResultEvicted: + { + var expired = MutationTransactions.SerializeFailure( + RequestedCoreMode(args), + preview: false, + SafeVersion(liveSession), + EditErrorCode.TransactionResultEvicted, + "the transaction is known, but its exact response has expired from bounded retention", + "transaction"); + return MutationTransactions.AttachIdentity(expired, identity); + } + case MutationTransactionDecisionKind.Reserved: + break; + default: + throw new InvalidOperationException("unknown mutation transaction decision"); + } + + var reservation = decision.Record!; + string terminalResponse; + try + { + terminalResponse = MutationTransactions.AttachIdentity( + ExecuteMutationRequest(liveSession, args, transactional: true), identity); + } + catch (Exception ex) + { + var callerError = ex is McpToolException + or FormatException or JsonException or OverflowException; + terminalResponse = MutationTransactions.AttachIdentity( + MutationTransactions.SerializeFailure( + RequestedCoreMode(args), + preview: false, + SafeVersion(liveSession), + callerError ? EditErrorCode.InvalidBatchStep : EditErrorCode.InternalError, + ex.Message, + callerError ? "validation" : "dispatch"), + identity); + } + liveSession.MutationTransactions.Complete(reservation, terminalResponse); + return terminalResponse; + } + + private static string ExecuteMutationRequest( + DocSession liveSession, + JsonElement args, + bool transactional) + { var mode = args.TryGetProperty("mode", out _) ? Str(args, "mode") : "atomic"; @@ -891,11 +994,63 @@ private static string Mutations(SessionStore store, JsonElement args) } var liveBatchCheck = Check(liveSession, ParsePreconditions(args, MutationTarget(args))); - if (liveBatchCheck is not null) return liveBatchCheck; + if (liveBatchCheck is not null) + { + if (!transactional) return liveBatchCheck; + var error = DocxSessionJson.DeserializeEditResults(liveBatchCheck) + .FirstOrDefault()?.Error + ?? new EditError(EditErrorCode.PreconditionFailed, + "batch precondition failed"); + return MutationTransactions.SerializeFailure( + coreMode, + preview: false, + SafeVersion(liveSession), + error, + "preconditions", + rolledBack: coreMode == MutationBatchMode.Atomic); + } var liveSteps = BuildMutationBatchSteps(liveSession, stepsEl, legacyApply: mode == "apply"); return DocxSessionOps.ExecuteBatch(liveSession.Handle, coreMode, liveSteps); } + private static string? TransactionId(JsonElement args) + { + if (args.ValueKind != JsonValueKind.Object + || !args.TryGetProperty("transactionId", out var value)) + return null; + if (value.ValueKind != JsonValueKind.String) + throw new McpToolException("transactionId must be a string"); + var id = value.GetString()!; + if (string.IsNullOrWhiteSpace(id)) + throw new McpToolException("transactionId must not be empty or whitespace"); + if (id.Length > MutationTransactions.MaxTransactionIdLength) + throw new McpToolException( + $"transactionId must not exceed {MutationTransactions.MaxTransactionIdLength} characters"); + return id; + } + + private static bool RequestsPreview(JsonElement args) => + args.ValueKind == JsonValueKind.Object + && ((args.TryGetProperty("mode", out var mode) + && mode.ValueKind == JsonValueKind.String + && mode.GetString() == "preview") + || (args.TryGetProperty("preview", out var preview) + && preview.ValueKind == JsonValueKind.True)); + + private static MutationBatchMode RequestedCoreMode(JsonElement args) => + args.ValueKind == JsonValueKind.Object + && args.TryGetProperty("mode", out var mode) + && mode.ValueKind == JsonValueKind.String + && mode.GetString() is "best_effort" or "apply" + ? MutationBatchMode.BestEffort + : MutationBatchMode.Atomic; + + private static long SafeVersion(DocSession session) + { + try { return DocxSessionOps.GetVersion(session.Handle); } + catch { return 0; } + } + private static IReadOnlyList BuildMutationBatchSteps( DocSession session, JsonElement stepsEl, @@ -985,6 +1140,11 @@ private static IReadOnlyList BuildMutationBatchSteps( string action, JsonElement args) { + if (args.TryGetProperty("transactionId", out _)) + return new EditError( + EditErrorCode.InvalidTransaction, + "mutation step args cannot contain transactionId; use the batch root"); + var actionError = ValidateMutationBatchAction(tool, action); if (actionError is not null) return actionError; diff --git a/tools/mcp-server/MutationTransactions.cs b/tools/mcp-server/MutationTransactions.cs new file mode 100644 index 00000000..e5fc6479 --- /dev/null +++ b/tools/mcp-server/MutationTransactions.cs @@ -0,0 +1,364 @@ +#nullable enable + +using System.Buffers; +using System.Security.Cryptography; +using System.Text; +using System.Text.Encodings.Web; +using System.Text.Json; +using Docxodus; +using Docxodus.Internal; + +namespace Docxodus.McpServer; + +/// The stable, versioned identity attached to a transaction-aware batch result. +internal sealed record MutationTransactionIdentity( + int SchemaVersion, + string TransactionId, + string RequestFingerprint); + +/// +/// One retained transaction result. Record ids and timestamps are server bookkeeping for tests, +/// diagnostics, and a future durable implementation; only is placed on +/// the MCP response in this in-session epic. +/// +internal sealed record MutationTransactionRecord( + string RecordId, + MutationTransactionIdentity Identity, + DateTimeOffset StartedAt, + DateTimeOffset? CompletedAt, + string? SerializedResponse); + +internal sealed record MutationTransactionTombstone( + string RecordId, + MutationTransactionIdentity Identity, + DateTimeOffset StartedAt, + DateTimeOffset CompletedAt, + DateTimeOffset EvictedAt); + +internal enum MutationTransactionDecisionKind +{ + Reserved, + Replay, + Conflict, + ResultEvicted, +} + +internal sealed record MutationTransactionDecision( + MutationTransactionDecisionKind Kind, + MutationTransactionRecord? Record = null, + MutationTransactionIdentity? ExistingIdentity = null, + string? SerializedResponse = null); + +/// +/// Bounded, per-session transaction-id registry. Full responses and response-less tombstones use +/// independent FIFO limits. A tombstone keeps an evicted id bound to its original fingerprint for +/// a further window, preventing a recently forgotten retry from becoming a fresh mutation. +/// +internal sealed class MutationTransactions +{ + public const int SchemaVersion = 1; + public const int DefaultFullRecordCapacity = 128; + public const int DefaultTombstoneCapacity = 1024; + public const int MaxTransactionIdLength = 256; + + private readonly int _fullRecordCapacity; + private readonly int _tombstoneCapacity; + private readonly Func _utcNow; + private readonly Func _recordIdFactory; + private readonly Dictionary _records = + new(StringComparer.Ordinal); + private readonly Queue _completedFifo = new(); + private readonly Dictionary _tombstones = + new(StringComparer.Ordinal); + private readonly Queue _tombstoneFifo = new(); + + public MutationTransactions( + int fullRecordCapacity = DefaultFullRecordCapacity, + int tombstoneCapacity = DefaultTombstoneCapacity, + Func? utcNow = null, + Func? recordIdFactory = null) + { + if (fullRecordCapacity < 1) + throw new ArgumentOutOfRangeException(nameof(fullRecordCapacity)); + if (tombstoneCapacity < 0) + throw new ArgumentOutOfRangeException(nameof(tombstoneCapacity)); + _fullRecordCapacity = fullRecordCapacity; + _tombstoneCapacity = tombstoneCapacity; + _utcNow = utcNow ?? (() => DateTimeOffset.UtcNow); + _recordIdFactory = recordIdFactory ?? NewRecordId; + } + + internal int FullRecordCount + { + get { lock (_records) return _completedFifo.Count; } + } + + internal int TombstoneCount + { + get { lock (_records) return _tombstones.Count; } + } + + internal MutationTransactionRecord? GetRecord(string transactionId) + { + lock (_records) + return _records.TryGetValue(transactionId, out var record) ? record : null; + } + + internal MutationTransactionTombstone? GetTombstone(string transactionId) + { + lock (_records) + return _tombstones.TryGetValue(transactionId, out var tombstone) ? tombstone : null; + } + + /// Reserve a new identity, or resolve it to replay/conflict/expired deterministically. + public MutationTransactionDecision Begin(string transactionId, string requestFingerprint) + { + var requested = new MutationTransactionIdentity( + SchemaVersion, transactionId, requestFingerprint); + lock (_records) + { + if (_records.TryGetValue(transactionId, out var record)) + { + if (!string.Equals(record.Identity.RequestFingerprint, requestFingerprint, + StringComparison.Ordinal)) + return new MutationTransactionDecision( + MutationTransactionDecisionKind.Conflict, + ExistingIdentity: record.Identity); + if (record.SerializedResponse is not null) + return new MutationTransactionDecision( + MutationTransactionDecisionKind.Replay, + record, + record.Identity, + record.SerializedResponse); + + // Per-session dispatch serialization means this cannot occur through Dispatcher; + // retaining a typed conflict makes the component safe if it is called directly. + return new MutationTransactionDecision( + MutationTransactionDecisionKind.Conflict, + ExistingIdentity: record.Identity); + } + + if (_tombstones.TryGetValue(transactionId, out var tombstone)) + { + return new MutationTransactionDecision( + string.Equals(tombstone.Identity.RequestFingerprint, requestFingerprint, + StringComparison.Ordinal) + ? MutationTransactionDecisionKind.ResultEvicted + : MutationTransactionDecisionKind.Conflict, + ExistingIdentity: tombstone.Identity); + } + + var reserved = new MutationTransactionRecord( + _recordIdFactory(), requested, _utcNow(), null, null); + _records.Add(transactionId, reserved); + return new MutationTransactionDecision( + MutationTransactionDecisionKind.Reserved, reserved, requested); + } + } + + /// Atomically retain the exact response and apply FIFO eviction. + public MutationTransactionRecord Complete( + MutationTransactionRecord reservation, + string serializedResponse) + { + ArgumentNullException.ThrowIfNull(reservation); + ArgumentNullException.ThrowIfNull(serializedResponse); + lock (_records) + { + if (!_records.TryGetValue(reservation.Identity.TransactionId, out var current) + || !ReferenceEquals(current, reservation) + || current.SerializedResponse is not null) + throw new InvalidOperationException("mutation transaction reservation is no longer active"); + + var completed = current with + { + CompletedAt = _utcNow(), + SerializedResponse = serializedResponse, + }; + _records[completed.Identity.TransactionId] = completed; + _completedFifo.Enqueue(completed.Identity.TransactionId); + EvictCompletedRecords(); + return completed; + } + } + + private void EvictCompletedRecords() + { + while (_completedFifo.Count > _fullRecordCapacity) + { + var id = _completedFifo.Dequeue(); + if (!_records.Remove(id, out var evicted) || evicted.CompletedAt is not { } completedAt) + continue; + if (_tombstoneCapacity == 0) continue; + + _tombstones[id] = new MutationTransactionTombstone( + evicted.RecordId, + evicted.Identity, + evicted.StartedAt, + completedAt, + _utcNow()); + _tombstoneFifo.Enqueue(id); + } + + while (_tombstoneFifo.Count > _tombstoneCapacity) + { + var id = _tombstoneFifo.Dequeue(); + _tombstones.Remove(id); + } + } + + /// + /// SHA-256 over a deterministic JSON rendering. Root session/transaction identity is excluded; + /// objects are sorted, arrays and scalar spelling are retained, and numeric tokens are copied + /// verbatim. Parsing already normalizes insignificant whitespace and equivalent string escapes. + /// + public static string Fingerprint(JsonElement request) + { + if (request.ValueKind != JsonValueKind.Object) + throw new McpToolException("docxodus_mutations arguments must be an object"); + + var buffer = new ArrayBufferWriter(); + using (var writer = new Utf8JsonWriter(buffer, new JsonWriterOptions + { + Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping, + })) + { + WriteCanonical(writer, request, isRoot: true, "$arguments"); + } + var hash = SHA256.HashData(buffer.WrittenSpan); + return "sha256:" + Convert.ToHexString(hash).ToLowerInvariant(); + } + + private static void WriteCanonical( + Utf8JsonWriter writer, + JsonElement value, + bool isRoot, + string path) + { + switch (value.ValueKind) + { + case JsonValueKind.Object: + { + var properties = value.EnumerateObject().ToArray(); + var names = new HashSet(StringComparer.Ordinal); + foreach (var property in properties) + { + if (!names.Add(property.Name)) + throw new McpToolException( + $"duplicate JSON property {JsonSerializer.Serialize(property.Name)} at {path}"); + } + Array.Sort(properties, static (left, right) => + StringComparer.Ordinal.Compare(left.Name, right.Name)); + + writer.WriteStartObject(); + var synthesizeAtomicMode = isRoot && !names.Contains("mode"); + foreach (var property in properties) + { + if (isRoot && property.Name is "sessionId" or "transactionId") continue; + if (synthesizeAtomicMode + && StringComparer.Ordinal.Compare("mode", property.Name) < 0) + { + writer.WriteString("mode", "atomic"); + synthesizeAtomicMode = false; + } + writer.WritePropertyName(property.Name); + WriteCanonical(writer, property.Value, isRoot: false, + path + "." + property.Name); + } + if (synthesizeAtomicMode) + writer.WriteString("mode", "atomic"); + writer.WriteEndObject(); + break; + } + case JsonValueKind.Array: + { + writer.WriteStartArray(); + var index = 0; + foreach (var item in value.EnumerateArray()) + { + WriteCanonical(writer, item, isRoot: false, $"{path}[{index}]"); + index++; + } + writer.WriteEndArray(); + break; + } + case JsonValueKind.String: + writer.WriteStringValue(value.GetString()); + break; + case JsonValueKind.Number: + writer.WriteRawValue(value.GetRawText(), skipInputValidation: true); + break; + case JsonValueKind.True: + writer.WriteBooleanValue(true); + break; + case JsonValueKind.False: + writer.WriteBooleanValue(false); + break; + case JsonValueKind.Null: + writer.WriteNullValue(); + break; + default: + throw new McpToolException($"unsupported JSON value at {path}"); + } + } + + /// Add the versioned identity to the already-serialized core batch result. + public static string AttachIdentity( + string serializedBatchResult, + MutationTransactionIdentity identity) + { + var end = serializedBatchResult.Length - 1; + while (end >= 0 && char.IsWhiteSpace(serializedBatchResult[end])) end--; + if (end < 1 || serializedBatchResult[0] != '{' || serializedBatchResult[end] != '}') + throw new InvalidOperationException("mutation batch result was not a JSON object"); + + var suffix = serializedBatchResult[(end + 1)..]; + return serializedBatchResult[..end] + + ",\"transaction\":{\"schemaVersion\":1,\"transactionId\":" + + JsonRpcIo.JsonString(identity.TransactionId) + + ",\"requestFingerprint\":" + + JsonRpcIo.JsonString(identity.RequestFingerprint) + + "}}" + + suffix; + } + + /// Serialize a transport-level terminal outcome through MutationBatchResult. + public static string SerializeFailure( + MutationBatchMode mode, + bool preview, + long version, + EditErrorCode code, + string message, + string action, + bool rolledBack = false) + => SerializeFailure( + mode, preview, version, new EditError(code, message), action, rolledBack); + + public static string SerializeFailure( + MutationBatchMode mode, + bool preview, + long version, + EditError error, + string action, + bool rolledBack = false) + { + var edit = new EditResult { Success = false, Error = error }; + var step = new MutationBatchStepResult( + 0, "docxodus_mutations", action, new[] { edit }, rolledBack); + return DocxSessionJson.SerializeMutationBatchResult(new MutationBatchResult + { + Mode = mode, + Preview = preview, + Success = false, + RolledBack = rolledBack, + BaseVersion = version, + ResultVersion = version, + Steps = new[] { step }, + Failure = new MutationBatchFailure( + step.Index, step.Tool, step.Action, error, rolledBack), + }); + } + + private static string NewRecordId() => + "mtx_" + Convert.ToHexString(RandomNumberGenerator.GetBytes(16)).ToLowerInvariant(); +} diff --git a/tools/mcp-server/SessionStore.cs b/tools/mcp-server/SessionStore.cs index a0405c59..13f178b5 100644 --- a/tools/mcp-server/SessionStore.cs +++ b/tools/mcp-server/SessionStore.cs @@ -16,6 +16,19 @@ internal sealed class DocSession required public string Id { get; init; } public int Handle { get; init; } + /// + /// Serializes every action addressed to this session. The core session has its own mutation + /// lock, but MCP dispatch also has to order reads, saves, closes, and transaction-id replay + /// decisions with mutations so a retry cannot race a different session action. + /// + internal object DispatchGate { get; } = new(); + + /// In-session mutation transaction identities and their exact serialized results. + internal MutationTransactions MutationTransactions { get; init; } = new(); + + /// False after close has won the dispatch race for this session. + internal bool Active { get; set; } = true; + /// Store-resolved location this session was opened from — already checked to be in /// scope, so a save back to it needs no re-validation. Null only if a session was opened from /// bytes with no origin. @@ -31,12 +44,21 @@ internal sealed class DocSession internal sealed class SessionStore { private readonly ConcurrentDictionary _sessions = new(); + private readonly object _lifecycleGate = new(); + private readonly System.Func _mutationTransactionsFactory; /// Backing document store. Defaults to a local store rooted at the /// process's current directory, which is only appropriate for tests — the server proper /// passes the environment-configured store from . - public SessionStore(IDocumentStore? documents = null) => - Documents = documents ?? new LocalFileDocumentStore(System.IO.Directory.GetCurrentDirectory()); + public SessionStore( + IDocumentStore? documents = null, + System.Func? mutationTransactionsFactory = null) + { + Documents = documents + ?? new LocalFileDocumentStore(System.IO.Directory.GetCurrentDirectory()); + _mutationTransactionsFactory = mutationTransactionsFactory + ?? (() => new MutationTransactions()); + } /// Where this server's documents are read from and written to. Every session in the /// process shares it, and it is already rooted at the configured scope. @@ -44,15 +66,19 @@ public SessionStore(IDocumentStore? documents = null) => public DocSession Open(byte[] bytes, string? location, DocxSessionSettings settings) { - var handle = DocxSessionOps.OpenSession(bytes, settings); - var session = new DocSession + lock (_lifecycleGate) { - Id = NewSessionId(), - Handle = handle, - Location = location, - }; - _sessions[session.Id] = session; - return session; + var handle = DocxSessionOps.OpenSession(bytes, settings); + var session = new DocSession + { + Id = NewSessionId(), + Handle = handle, + Location = location, + MutationTransactions = _mutationTransactionsFactory(), + }; + _sessions[session.Id] = session; + return session; + } } /// @@ -66,22 +92,60 @@ private static string NewSessionId() => public DocSession Get(string sessionId) { - if (!_sessions.TryGetValue(sessionId, out var session)) + if (!_sessions.TryGetValue(sessionId, out var session) || !session.Active) throw new McpToolException($"unknown session_id: {sessionId}"); return session; } + /// + /// Run one complete session-bound dispatch while holding the session's synchronous gate. + /// The active check after taking the gate closes the lookup/close race: a caller that found + /// the session before close removed it still cannot enter the disposed core handle. + /// + public string Dispatch(string sessionId, System.Func action) + { + if (!_sessions.TryGetValue(sessionId, out var session)) + throw new McpToolException($"unknown session_id: {sessionId}"); + lock (session.DispatchGate) + { + if (!session.Active + || !_sessions.TryGetValue(sessionId, out var current) + || !ReferenceEquals(session, current)) + throw new McpToolException($"unknown session_id: {sessionId}"); + return action(); + } + } + public void Close(string sessionId) { - if (_sessions.TryRemove(sessionId, out var session)) - DocxSessionOps.CloseSession(session.Handle); + lock (_lifecycleGate) + { + if (!_sessions.TryGetValue(sessionId, out var session)) return; + lock (session.DispatchGate) + { + if (!session.Active) return; + session.Active = false; + _sessions.TryRemove(sessionId, out _); + DocxSessionOps.CloseSession(session.Handle); + } + } } public void CloseAll() { - foreach (var kv in _sessions) - DocxSessionOps.CloseSession(kv.Value.Handle); - _sessions.Clear(); + lock (_lifecycleGate) + { + foreach (var kv in _sessions) + { + lock (kv.Value.DispatchGate) + { + if (!kv.Value.Active) continue; + kv.Value.Active = false; + _sessions.TryRemove(kv.Key, out _); + DocxSessionOps.CloseSession(kv.Value.Handle); + } + } + } } } diff --git a/tools/mcp-server/ToolCatalog.cs b/tools/mcp-server/ToolCatalog.cs index 9fae7be9..592d523e 100644 --- a/tools/mcp-server/ToolCatalog.cs +++ b/tools/mcp-server/ToolCatalog.cs @@ -502,12 +502,13 @@ internal static class ToolCatalog """), new ToolDefinition( "docxodus_mutations", - "Apply or safely preview a batch of mutating edit/format/create/table/list/comment/link/image/content-control/track-changes actions. Atomic mode commits as one unit; preview executes the identical batch path against an isolated complete package clone and never mutates the live session or its undo/redo history.", + "Apply or safely preview a batch of mutating edit/format/create/table/list/comment/link/image/content-control/track-changes actions. Atomic mode commits as one unit. An optional transactionId makes applying retries idempotent within this open session; preview is isolated and cannot carry a transactionId.", """ { "type": "object", "properties": { "sessionId": { "type": "string" }, + "transactionId": { "type": "string", "minLength": 1, "maxLength": 256, "description": "Optional caller identity for an APPLYING batch only. The first terminal response is retained in this open session; an identical retry returns that exact serialized response without executing or rechecking preconditions. Reusing the id for a different canonical request returns transaction_conflict. Preview/dry-run rejects this field." }, "preconditions": { "type": "object", "description": "Optional batch-start guards. Each step args object may also carry its own preconditions." }, "mode": { "type": "string", "enum": ["atomic", "best_effort", "apply", "preview"], "default": "atomic", "description": "atomic (default): all steps commit as one undo/version unit or fully roll back. best_effort: explicitly retain successful steps after failures. apply: deprecated alias for best_effort. preview: isolated dry-run shorthand using atomic policy unless previewPolicy says best_effort." }, "preview": { "type": "boolean", "default": false, "description": "Dry-run mode for mode=atomic or mode=best_effort. The complete package is cloned and the live document, version, caches, configuration, and undo/redo history are never touched." }, @@ -520,7 +521,7 @@ internal static class ToolCatalog "type": "object", "properties": { "tool": { "type": "string", "enum": ["docxodus_edit", "docxodus_format", "docxodus_create", "docxodus_table", "docxodus_list", "docxodus_comment", "docxodus_links", "docxodus_images", "docxodus_content_controls", "docxodus_track_changes"] }, - "args": { "type": "object", "description": "The same arguments that tool's action takes, minus sessionId (inherited from the batch)." } + "args": { "type": "object", "description": "The same arguments that tool's action takes, minus sessionId (inherited from the batch). transactionId is forbidden here; it belongs only at the batch root." } }, "required": ["tool", "args"] } From 92aa2e04a2daf763aaf985d9c409bf3ee2a5176c Mon Sep 17 00:00:00 2001 From: JSv4 Date: Fri, 14 Aug 2026 07:03:05 -0500 Subject: [PATCH 2/5] Test and document mutation transaction identity --- Docxodus.Tests/McpMutationTransactionTests.cs | 677 ++++++++++++++++++ docs/architecture/docx_agent_server.md | 34 + tools/mcp-server/README.md | 11 + 3 files changed, 722 insertions(+) create mode 100644 Docxodus.Tests/McpMutationTransactionTests.cs diff --git a/Docxodus.Tests/McpMutationTransactionTests.cs b/Docxodus.Tests/McpMutationTransactionTests.cs new file mode 100644 index 00000000..07b43326 --- /dev/null +++ b/Docxodus.Tests/McpMutationTransactionTests.cs @@ -0,0 +1,677 @@ +#nullable enable + +using System.Collections.Concurrent; +using System.Text.Json; +using Docxodus.McpServer; +using Xunit; + +namespace Docxodus.Tests; + +/// Issue #449: in-session idempotency and session-wide dispatch ordering. +public sealed class McpMutationTransactionTests : IDisposable +{ + private readonly string _root; + private readonly string _path; + private readonly SessionStore _store; + + public McpMutationTransactionTests() + { + _root = Path.Combine(Path.GetTempPath(), $"mcp-transactions-{Guid.NewGuid():N}"); + Directory.CreateDirectory(_root); + _path = Path.Combine(_root, "document.docx"); + File.WriteAllBytes(_path, DocxSession.CreateBlankDocxBytes()); + _store = new SessionStore(new LocalFileDocumentStore(_root)); + } + + public void Dispose() + { + _store.CloseAll(); + if (Directory.Exists(_root)) Directory.Delete(_root, recursive: true); + } + + [Fact] + public void MCP449_ResponseLossRetryIsByteExactAndDoesNotDisturbUndoRedo() + { + var sessionId = OpenSession(_store, _path); + var anchor = FirstAnchor(_store, sessionId); + var args = MutationArgs(sessionId, "tx-response-loss", anchor, "inserted exactly once"); + + // Simulate transport loss by discarding the first returned string. + var original = Dispatcher.Call(_store, "docxodus_mutations", J(args)); + var parsed = J(original); + Assert.True(parsed.GetProperty("success").GetBoolean()); + Assert.False(parsed.GetProperty("rolledBack").GetBoolean()); + Assert.Equal("ok", parsed.GetProperty("status").GetString()); + Assert.Equal(0, parsed.GetProperty("baseVersion").GetInt64()); + Assert.Equal(1, parsed.GetProperty("resultVersion").GetInt64()); + Assert.NotEmpty(parsed.GetProperty("packageHash").GetString()!); + var step = Assert.Single(parsed.GetProperty("steps").EnumerateArray()); + Assert.Equal("docxodus_edit", step.GetProperty("tool").GetString()); + Assert.Equal("insert_paragraph", step.GetProperty("action").GetString()); + var edit = Assert.Single(step.GetProperty("results").EnumerateArray()); + Assert.NotEmpty(edit.GetProperty("created")[0].GetProperty("id").GetString()!); + var identity = parsed.GetProperty("transaction"); + Assert.Equal(1, identity.GetProperty("schemaVersion").GetInt32()); + Assert.Equal("tx-response-loss", identity.GetProperty("transactionId").GetString()); + Assert.StartsWith("sha256:", identity.GetProperty("requestFingerprint").GetString()); + var retained = Assert.IsType( + _store.Get(sessionId).MutationTransactions.GetRecord("tx-response-loss")); + Assert.Equal(original, retained.SerializedResponse); + var retainedResult = J(retained.SerializedResponse!); + Assert.Equal(0, retainedResult.GetProperty("baseVersion").GetInt64()); + Assert.Equal(1, retainedResult.GetProperty("resultVersion").GetInt64()); + Assert.True(retainedResult.GetProperty("success").GetBoolean()); + Assert.False(retainedResult.GetProperty("rolledBack").GetBoolean()); + Assert.NotEmpty(retainedResult.GetProperty("packageHash").GetString()!); + var retainedStep = Assert.Single(retainedResult.GetProperty("steps").EnumerateArray()); + Assert.Equal("docxodus_edit", retainedStep.GetProperty("tool").GetString()); + Assert.Equal("insert_paragraph", retainedStep.GetProperty("action").GetString()); + Assert.NotEmpty(Assert.Single(retainedStep.GetProperty("results").EnumerateArray()) + .GetProperty("created")[0].GetProperty("id").GetString()!); + + Assert.True(J(Dispatcher.Call(_store, "docxodus_edit", J(JsonSerializer.Serialize(new + { + sessionId, + action = "undo", + })))).GetProperty("success").GetBoolean()); + + var replay = Dispatcher.Call(_store, "docxodus_mutations", J(args)); + Assert.Equal(original, replay); + Assert.True(J(Dispatcher.Call(_store, "docxodus_edit", J(JsonSerializer.Serialize(new + { + sessionId, + action = "redo", + })))).GetProperty("success").GetBoolean()); + var markdown = GetMarkdown(_store, sessionId); + Assert.Equal(1, Occurrences(markdown, "inserted exactly once")); + } + + [Fact] + public void MCP449_OmittedAtomicModeAndExplicitAtomicModeReplayExactly() + { + var sessionId = OpenSession(_store, _path); + var anchor = FirstAnchor(_store, sessionId); + var omitted = MutationArgs(sessionId, "tx-default-mode", anchor, "default mode"); + var explicitAtomic = omitted.Replace( + "\"transactionId\":\"tx-default-mode\",", + "\"mode\":\"atomic\",\"transactionId\":\"tx-default-mode\","); + + var first = Dispatcher.Call(_store, "docxodus_mutations", J(omitted)); + var second = Dispatcher.Call(_store, "docxodus_mutations", J(explicitAtomic)); + + Assert.Equal(first, second); + Assert.Equal(1, Docxodus.Internal.DocxSessionOps.GetVersion(_store.Get(sessionId).Handle)); + } + + [Fact] + public void MCP449_SameIdDifferentRequestReturnsTypedConflictWithoutMutation() + { + var sessionId = OpenSession(_store, _path); + var anchor = FirstAnchor(_store, sessionId); + var first = Dispatcher.Call(_store, "docxodus_mutations", + J(MutationArgs(sessionId, "tx-conflict", anchor, "first"))); + var conflict = J(Dispatcher.Call(_store, "docxodus_mutations", + J(MutationArgs(sessionId, "tx-conflict", anchor, "second")))); + + Assert.True(J(first).GetProperty("success").GetBoolean()); + Assert.Equal("transaction_conflict", + conflict.GetProperty("failure").GetProperty("error").GetProperty("code").GetString()); + Assert.Equal(1, Docxodus.Internal.DocxSessionOps.GetVersion(_store.Get(sessionId).Handle)); + Assert.DoesNotContain("second", GetMarkdown(_store, sessionId)); + } + + [Fact] + public void MCP449_AtomicRollbackAndBestEffortPartialResultsAreCached() + { + var atomicSession = OpenSession(_store, _path); + var atomicAnchor = FirstAnchor(_store, atomicSession); + var atomicArgs = JsonSerializer.Serialize(new + { + sessionId = atomicSession, + transactionId = "tx-atomic-failure", + mode = "atomic", + steps = new object[] + { + Step("replace_text", atomicAnchor, "speculative"), + Step("replace_text", "p:body:missing", "fail"), + }, + }); + var atomic = Dispatcher.Call(_store, "docxodus_mutations", J(atomicArgs)); + Assert.Equal(atomic, Dispatcher.Call(_store, "docxodus_mutations", J(atomicArgs))); + var atomicResult = J(atomic); + Assert.Equal("failed", atomicResult.GetProperty("status").GetString()); + Assert.True(atomicResult.GetProperty("rolledBack").GetBoolean()); + Assert.Equal(0, Docxodus.Internal.DocxSessionOps.GetVersion(_store.Get(atomicSession).Handle)); + + var partialSession = OpenSession(_store, _path); + var partialAnchor = FirstAnchor(_store, partialSession); + var partialArgs = JsonSerializer.Serialize(new + { + sessionId = partialSession, + transactionId = "tx-partial", + mode = "best_effort", + steps = new object[] + { + Step("replace_text", partialAnchor, "retained partial"), + Step("replace_text", "p:body:missing", "fail"), + }, + }); + var partial = Dispatcher.Call(_store, "docxodus_mutations", J(partialArgs)); + Assert.Equal(partial, Dispatcher.Call(_store, "docxodus_mutations", J(partialArgs))); + Assert.Equal("partial", J(partial).GetProperty("status").GetString()); + Assert.Equal(1, Docxodus.Internal.DocxSessionOps.GetVersion(_store.Get(partialSession).Handle)); + Assert.Contains("retained partial", GetMarkdown(_store, partialSession)); + } + + [Fact] + public void MCP449_PreconditionAndValidationFailuresReplayBeforeCurrentStateChecks() + { + var sessionId = OpenSession(_store, _path); + var anchor = FirstAnchor(_store, sessionId); + var guardedArgs = JsonSerializer.Serialize(new + { + sessionId, + transactionId = "tx-precondition", + preconditions = new { expectedVersion = 99 }, + steps = new[] { Step("replace_text", anchor, "must not apply") }, + }); + var failed = Dispatcher.Call(_store, "docxodus_mutations", J(guardedArgs)); + Assert.Equal("precondition_failed", J(failed).GetProperty("failure") + .GetProperty("error").GetProperty("code").GetString()); + + ReplaceDirect(_store, sessionId, anchor, "later state"); + Assert.Equal(failed, Dispatcher.Call(_store, "docxodus_mutations", J(guardedArgs))); + Assert.Contains("later state", GetMarkdown(_store, sessionId)); + + var invalidArgs = $$""" + {"sessionId":{{JsonSerializer.Serialize(sessionId)}},"transactionId":"tx-validation","mode":"sideways","steps":[]} + """; + var invalid = Dispatcher.Call(_store, "docxodus_mutations", J(invalidArgs)); + Assert.Equal("invalid_batch_step", J(invalid).GetProperty("failure") + .GetProperty("error").GetProperty("code").GetString()); + Assert.Equal(invalid, Dispatcher.Call(_store, "docxodus_mutations", J(invalidArgs))); + } + + [Fact] + public void MCP449_PreviewDryRunDirectToolsAndStepArgsRejectTransactionIds() + { + var sessionId = OpenSession(_store, _path); + var anchor = FirstAnchor(_store, sessionId); + foreach (var previewProperties in new[] + { + "\"mode\":\"preview\"", + "\"mode\":\"atomic\",\"preview\":true", + }) + { + var args = "{\"sessionId\":" + JsonSerializer.Serialize(sessionId) + + ",\"transactionId\":\"tx-preview\"," + previewProperties + + ",\"steps\":[{\"tool\":\"docxodus_edit\",\"args\":{\"action\":\"replace_text\"" + + ",\"anchorId\":" + JsonSerializer.Serialize(anchor) + + ",\"markdown\":\"shadow\"}}]}"; + var rejected = J(Dispatcher.Call(_store, "docxodus_mutations", J(args))); + Assert.Equal("invalid_transaction", rejected.GetProperty("failure") + .GetProperty("error").GetProperty("code").GetString()); + Assert.False(rejected.TryGetProperty("transaction", out _)); + } + Assert.Equal(0, _store.Get(sessionId).MutationTransactions.FullRecordCount); + + Assert.Throws(() => Dispatcher.Call(_store, "docxodus_edit", + J(JsonSerializer.Serialize(new + { + sessionId, + transactionId = "nested-direct", + action = "replace_text", + anchorId = anchor, + markdown = "no", + })))); + + var nested = J(Dispatcher.Call(_store, "docxodus_mutations", J(JsonSerializer.Serialize(new + { + sessionId, + steps = new[] + { + new + { + tool = "docxodus_edit", + args = new + { + transactionId = "nested-step", + action = "replace_text", + anchorId = anchor, + markdown = "no", + }, + }, + }, + })))); + Assert.Equal("invalid_transaction", nested.GetProperty("failure") + .GetProperty("error").GetProperty("code").GetString()); + } + + [Fact] + public void MCP449_CanonicalFingerprintNormalizesObjectsWhitespaceAndEscapesOnly() + { + var left = J(""" + { + "sessionId": "session-a", + "transactionId": "tx-a", + "unknown": { "z": "\u0061", "a": true }, + "steps": [1, { "right": null, "left": "same" }] + } + """); + var right = J("""{"steps":[1,{"left":"same","right":null}],"unknown":{"a":true,"z":"a"},"mode":"atomic","transactionId":"tx-b","sessionId":"session-b"}"""); + Assert.Equal( + MutationTransactions.Fingerprint(left), + MutationTransactions.Fingerprint(right)); + + var baseline = MutationTransactions.Fingerprint(J("""{"steps":[],"mode":"atomic","unknown":1}""")); + Assert.NotEqual(baseline, + MutationTransactions.Fingerprint(J("""{"steps":[],"mode":"atomic","unknown":1.0}"""))); + Assert.NotEqual(baseline, + MutationTransactions.Fingerprint(J("""{"steps":[],"mode":"atomic","unknown":"1"}"""))); + Assert.NotEqual( + MutationTransactions.Fingerprint(J("""{"steps":[],"unknown":"Spelling"}""")), + MutationTransactions.Fingerprint(J("""{"steps":[],"unknown":"spelling"}"""))); + Assert.NotEqual(baseline, + MutationTransactions.Fingerprint(J("""{"steps":[],"mode":"atomic","unknown":1,"extra":null}"""))); + Assert.NotEqual( + MutationTransactions.Fingerprint(J("""{"steps":[1,2]}""")), + MutationTransactions.Fingerprint(J("""{"steps":[2,1]}"""))); + Assert.NotEqual( + MutationTransactions.Fingerprint(J("""{"steps":[],"mode":"apply"}""")), + MutationTransactions.Fingerprint(J("""{"steps":[],"mode":"best_effort"}"""))); + Assert.NotEqual( + MutationTransactions.Fingerprint(J("""{"steps":[],"preview":false}""")), + MutationTransactions.Fingerprint(J("""{"steps":[]}"""))); + } + + [Fact] + public void MCP449_DuplicateKeysAtAnyDepthAreRejectedBeforeReservation() + { + var sessionId = OpenSession(_store, _path); + var duplicate = "{\"sessionId\":" + JsonSerializer.Serialize(sessionId) + + ",\"transactionId\":\"tx-duplicate\",\"steps\":[{\"tool\":\"docxodus_edit\"" + + ",\"args\":{\"action\":\"replace_text\",\"action\":\"replace_text\"}}]}"; + + var error = Assert.Throws(() => + Dispatcher.Call(_store, "docxodus_mutations", J(duplicate))); + Assert.Contains("duplicate JSON property", error.Message, StringComparison.Ordinal); + Assert.Equal(0, _store.Get(sessionId).MutationTransactions.FullRecordCount); + Assert.Null(_store.Get(sessionId).MutationTransactions.GetRecord("tx-duplicate")); + } + + [Fact] + public void MCP449_JournalUsesGeneratedMetadataAndBoundedFullThenTombstoneFifos() + { + var now = new DateTimeOffset(2026, 8, 14, 12, 0, 0, TimeSpan.Zero); + var recordNumber = 0; + var journal = new MutationTransactions( + fullRecordCapacity: 1, + tombstoneCapacity: 1, + utcNow: () => now, + recordIdFactory: () => $"record-{++recordNumber}"); + + var a = AssertReserved(journal.Begin("a", "sha256:a")); + Assert.Equal("record-1", a.RecordId); + Assert.Equal(now, a.StartedAt); + now = now.AddSeconds(1); + var completedA = journal.Complete(a, "{\"a\":1}"); + Assert.Equal(now, completedA.CompletedAt); + + var b = AssertReserved(journal.Begin("b", "sha256:b")); + now = now.AddSeconds(1); + journal.Complete(b, "{\"b\":1}"); + Assert.Equal(1, journal.FullRecordCount); + Assert.Equal(1, journal.TombstoneCount); + Assert.Equal(MutationTransactionDecisionKind.ResultEvicted, + journal.Begin("a", "sha256:a").Kind); + Assert.Equal(MutationTransactionDecisionKind.Conflict, + journal.Begin("a", "sha256:different").Kind); + + var c = AssertReserved(journal.Begin("c", "sha256:c")); + now = now.AddSeconds(1); + journal.Complete(c, "{\"c\":1}"); + Assert.Null(journal.GetTombstone("a")); + Assert.Equal(MutationTransactionDecisionKind.Reserved, + journal.Begin("a", "sha256:fresh-after-both-fifos").Kind); + } + + [Fact] + public void MCP449_DispatcherSerializesEvictedResultAndConflictAndReusesOnlyAfterTombstone() + { + using var store = new TestSessionStore( + new LocalFileDocumentStore(_root), + () => new MutationTransactions(fullRecordCapacity: 1, tombstoneCapacity: 1)); + var sessionId = OpenSession(store.Value, _path); + var anchor = FirstAnchor(store.Value, sessionId); + + string Args(string id, string markdown) => JsonSerializer.Serialize(new + { + sessionId, + transactionId = id, + steps = new[] { Step("replace_text", anchor, markdown) }, + }); + + var firstA = Dispatcher.Call(store.Value, "docxodus_mutations", J(Args("a", "a1"))); + Dispatcher.Call(store.Value, "docxodus_mutations", J(Args("b", "b1"))); + + var evicted = J(Dispatcher.Call( + store.Value, "docxodus_mutations", J(Args("a", "a1")))); + Assert.Equal("transaction_result_evicted", evicted.GetProperty("failure") + .GetProperty("error").GetProperty("code").GetString()); + var conflict = J(Dispatcher.Call( + store.Value, "docxodus_mutations", J(Args("a", "different")))); + Assert.Equal("transaction_conflict", conflict.GetProperty("failure") + .GetProperty("error").GetProperty("code").GetString()); + Assert.Equal(2, Docxodus.Internal.DocxSessionOps.GetVersion(store.Value.Get(sessionId).Handle)); + + Dispatcher.Call(store.Value, "docxodus_mutations", J(Args("c", "c1"))); + var reused = Dispatcher.Call( + store.Value, "docxodus_mutations", J(Args("a", "fresh after tombstone"))); + Assert.NotEqual(firstA, reused); + Assert.True(J(reused).GetProperty("success").GetBoolean()); + Assert.Equal(4, Docxodus.Internal.DocxSessionOps.GetVersion(store.Value.Get(sessionId).Handle)); + } + + [Fact] + public void MCP449_UnexpectedPostReservationFailureIsStructuredAndCached() + { + var sessionId = OpenSession(_store, _path); + var anchor = FirstAnchor(_store, sessionId); + var args = MutationArgs(sessionId, "tx-unexpected", anchor, "cannot execute"); + + // Invalidate only the lower-level handle while leaving the MCP session registered. This + // forces an unexpected registry failure after transaction reservation. + Docxodus.Internal.DocxSessionOps.CloseSession(_store.Get(sessionId).Handle); + var first = Dispatcher.Call(_store, "docxodus_mutations", J(args)); + var failure = J(first); + Assert.Equal("internal_error", failure.GetProperty("failure") + .GetProperty("error").GetProperty("code").GetString()); + Assert.Equal(first, Dispatcher.Call(_store, "docxodus_mutations", J(args))); + Assert.Equal(1, _store.Get(sessionId).MutationTransactions.FullRecordCount); + } + + [Fact] + public void MCP449_GeneratedRevisionTimestampAndIdentityReplayExactly() + { + var sessionId = OpenSession(_store, _path); + var anchor = FirstAnchor(_store, sessionId); + ReplaceDirect(_store, sessionId, anchor, "revision target"); + Dispatcher.Call(_store, "docxodus_track_changes", J(JsonSerializer.Serialize(new + { + sessionId, + action = "set_mode", + mode = "render_inline", + revisionAuthor = "Reviewer", + }))); + var args = JsonSerializer.Serialize(new + { + sessionId, + transactionId = "tx-generated-revision", + steps = new[] + { + new + { + tool = "docxodus_edit", + args = new + { + action = "replace_text", + anchorId = anchor, + markdown = "Generated metadata", + }, + }, + }, + }); + + var original = Dispatcher.Call(_store, "docxodus_mutations", J(args)); + var revisions = J(original).GetProperty("revisionChanges") + .GetProperty("added").EnumerateArray().ToArray(); + Assert.NotEmpty(revisions); + Assert.All(revisions, revision => + { + Assert.NotEmpty(revision.GetProperty("id").GetString()!); + Assert.False(string.IsNullOrWhiteSpace(revision.GetProperty("date").GetString())); + }); + Assert.Equal(original, Dispatcher.Call(_store, "docxodus_mutations", J(args))); + } + + [Fact] + public void MCP449_TransactionIdsAreSessionScopedAndCloseClearsTheirLifecycle() + { + var firstSession = OpenSession(_store, _path); + var secondSession = OpenSession(_store, _path); + var firstResult = Dispatcher.Call(_store, "docxodus_mutations", + J(MutationArgs(firstSession, "same-id", FirstAnchor(_store, firstSession), "first session"))); + var secondResult = Dispatcher.Call(_store, "docxodus_mutations", + J(MutationArgs(secondSession, "same-id", FirstAnchor(_store, secondSession), "second session"))); + Assert.True(J(firstResult).GetProperty("success").GetBoolean()); + Assert.True(J(secondResult).GetProperty("success").GetBoolean()); + + _store.Close(firstSession); + Assert.Throws(() => Dispatcher.Call(_store, "docxodus_mutations", + J(MutationArgs(firstSession, "same-id", "p:body:any", "retry after close")))); + Assert.Contains("second session", GetMarkdown(_store, secondSession)); + } + + [Fact] + public void MCP449_ConcurrentIdenticalCallsSerializeToOneMutationAndOneExactReplay() + { + var sessionId = OpenSession(_store, _path); + var args = J(MutationArgs( + sessionId, "tx-concurrent", FirstAnchor(_store, sessionId), "concurrent once")); + var start = new ManualResetEventSlim(false); + var results = new ConcurrentBag(); + var calls = Enumerable.Range(0, 8).Select(_ => Task.Run(() => + { + start.Wait(); + results.Add(Dispatcher.Call(_store, "docxodus_mutations", args)); + })).ToArray(); + + start.Set(); + Task.WaitAll(calls); + Assert.Equal(8, results.Count); + Assert.Single(results.Distinct(StringComparer.Ordinal)); + Assert.Equal(1, Docxodus.Internal.DocxSessionOps.GetVersion(_store.Get(sessionId).Handle)); + Assert.Equal(1, Occurrences(GetMarkdown(_store, sessionId), "concurrent once")); + } + + [Fact] + public void MCP449_SaveCloseAndCloseAllWaitForTheSameSessionDispatchGate() + { + var blockingStore = new BlockingDocumentStore(DocxSession.CreateBlankDocxBytes()); + var store = new SessionStore(blockingStore); + try + { + var sessionId = OpenSession(store, "document.docx"); + var save = Task.Run(() => Dispatcher.Call(store, "docxodus_save", + J(JsonSerializer.Serialize(new { sessionId })))); + Assert.True(blockingStore.WriteEntered.Wait(TimeSpan.FromSeconds(5))); + var close = Task.Run(() => Dispatcher.Call(store, "docxodus_close", + J(JsonSerializer.Serialize(new { sessionId })))); + Assert.False(close.Wait(TimeSpan.FromMilliseconds(100))); + blockingStore.ReleaseWrite.Set(); + Assert.True(save.Wait(TimeSpan.FromSeconds(5))); + Assert.True(close.Wait(TimeSpan.FromSeconds(5))); + + var closeAllSession = OpenSession(store, "document.docx"); + var entered = new ManualResetEventSlim(false); + var release = new ManualResetEventSlim(false); + var action = Task.Run(() => store.Dispatch(closeAllSession, () => + { + entered.Set(); + release.Wait(); + return "{}"; + })); + Assert.True(entered.Wait(TimeSpan.FromSeconds(5))); + var closeAll = Task.Run(store.CloseAll); + Assert.False(closeAll.Wait(TimeSpan.FromMilliseconds(100))); + release.Set(); + Assert.True(action.Wait(TimeSpan.FromSeconds(5))); + Assert.True(closeAll.Wait(TimeSpan.FromSeconds(5))); + Assert.Throws(() => store.Get(closeAllSession)); + } + finally + { + blockingStore.ReleaseWrite.Set(); + store.CloseAll(); + } + } + + [Fact] + public void MCP449_SaveThenRetryPreservesTheSavedMutationWithoutApplyingAgain() + { + var sessionId = OpenSession(_store, _path); + var args = MutationArgs( + sessionId, "tx-save", FirstAnchor(_store, sessionId), "saved transaction"); + var original = Dispatcher.Call(_store, "docxodus_mutations", J(args)); + Dispatcher.Call(_store, "docxodus_save", J(JsonSerializer.Serialize(new { sessionId }))); + Assert.Equal(original, Dispatcher.Call(_store, "docxodus_mutations", J(args))); + Assert.Equal(1, Docxodus.Internal.DocxSessionOps.GetVersion(_store.Get(sessionId).Handle)); + + Dispatcher.Call(_store, "docxodus_close", J(JsonSerializer.Serialize(new { sessionId }))); + var reopened = OpenSession(_store, _path); + Assert.Contains("saved transaction", GetMarkdown(_store, reopened)); + var reused = Dispatcher.Call(_store, "docxodus_mutations", J(MutationArgs( + reopened, "tx-save", FirstAnchor(_store, reopened), "fresh identity after reopen"))); + Assert.True(J(reused).GetProperty("success").GetBoolean()); + Assert.Equal(1, Docxodus.Internal.DocxSessionOps.GetVersion(_store.Get(reopened).Handle)); + Assert.Contains("fresh identity after reopen", GetMarkdown(_store, reopened)); + } + + [Fact] + public void MCP449_ToolSchemaDocumentsBoundedApplyingTransactionIdentity() + { + var tool = Assert.Single(ToolCatalog.Tools, + candidate => candidate.Name == "docxodus_mutations"); + var schema = J(tool.InputSchemaJson); + var transactionId = schema.GetProperty("properties").GetProperty("transactionId"); + Assert.Equal("string", transactionId.GetProperty("type").GetString()); + Assert.Equal(1, transactionId.GetProperty("minLength").GetInt32()); + Assert.Equal(MutationTransactions.MaxTransactionIdLength, + transactionId.GetProperty("maxLength").GetInt32()); + Assert.Contains("APPLYING", transactionId.GetProperty("description").GetString(), + StringComparison.Ordinal); + Assert.Equal(128, MutationTransactions.DefaultFullRecordCapacity); + Assert.Equal(1024, MutationTransactions.DefaultTombstoneCapacity); + } + + private static MutationTransactionRecord AssertReserved(MutationTransactionDecision decision) + { + Assert.Equal(MutationTransactionDecisionKind.Reserved, decision.Kind); + return Assert.IsType(decision.Record); + } + + private static object Step(string action, string anchorId, string markdown) => new + { + tool = "docxodus_edit", + args = new { action, anchorId, markdown }, + }; + + private static string MutationArgs( + string sessionId, + string transactionId, + string anchorId, + string markdown) => JsonSerializer.Serialize(new + { + sessionId, + transactionId, + steps = new[] + { + new + { + tool = "docxodus_edit", + args = new + { + action = "insert_paragraph", + anchorId, + position = "after", + markdown, + }, + }, + }, + }); + + private static string OpenSession(SessionStore store, string path) + { + var opened = J(Dispatcher.Call(store, "docxodus_open", + J(JsonSerializer.Serialize(new { path })))); + return opened.GetProperty("sessionId").GetString()!; + } + + private static string FirstAnchor(SessionStore store, string sessionId) + { + var content = J(Dispatcher.Call(store, "docxodus_get_content", + J(JsonSerializer.Serialize(new { sessionId, format = "markdown" })))); + return content.GetProperty("anchorIndex").EnumerateObject().First().Name; + } + + private static string GetMarkdown(SessionStore store, string sessionId) => + J(Dispatcher.Call(store, "docxodus_get_content", + J(JsonSerializer.Serialize(new { sessionId, format = "markdown" })))) + .GetProperty("markdown").GetString()!; + + private static void ReplaceDirect( + SessionStore store, + string sessionId, + string anchorId, + string markdown) + { + var result = J(Dispatcher.Call(store, "docxodus_edit", J(JsonSerializer.Serialize(new + { + sessionId, + action = "replace_text", + anchorId, + markdown, + })))); + Assert.True(result.GetProperty("success").GetBoolean()); + } + + private static int Occurrences(string value, string search) + { + var count = 0; + var index = 0; + while ((index = value.IndexOf(search, index, StringComparison.Ordinal)) >= 0) + { + count++; + index += search.Length; + } + return count; + } + + private static JsonElement J(string json) + { + using var document = JsonDocument.Parse(json); + return document.RootElement.Clone(); + } + + private sealed class BlockingDocumentStore : IDocumentStore + { + private readonly byte[] _bytes; + + public BlockingDocumentStore(byte[] bytes) => _bytes = bytes; + + public string Kind => "blocking-test"; + public string RootDescription => "blocking-test"; + public ManualResetEventSlim WriteEntered { get; } = new(false); + public ManualResetEventSlim ReleaseWrite { get; } = new(false); + public string Resolve(string location) => location; + public byte[] Read(string resolvedLocation) => _bytes.ToArray(); + + public void Write(string resolvedLocation, byte[] bytes) + { + WriteEntered.Set(); + ReleaseWrite.Wait(); + } + } + + private sealed class TestSessionStore : IDisposable + { + public TestSessionStore( + IDocumentStore documents, + Func journalFactory) => + Value = new SessionStore(documents, journalFactory); + + public SessionStore Value { get; } + + public void Dispose() => Value.CloseAll(); + } +} diff --git a/docs/architecture/docx_agent_server.md b/docs/architecture/docx_agent_server.md index f11f8de0..b5b88e09 100644 --- a/docs/architecture/docx_agent_server.md +++ b/docs/architecture/docx_agent_server.md @@ -498,6 +498,40 @@ modulo those generated ids/timestamps. Such receipts carry warnings; clients mus id or `packageHash` equality unless the operation supplies stable ids/timestamps or is otherwise known deterministic. +Applying batches may carry a caller-chosen root `transactionId` (a non-empty string up to 256 +characters). Its first terminal success, partial result, structured failure, precondition failure, +or safely-caught exception is recorded for the lifetime of that open session. An identical retry +returns the original serialized `MutationBatchResult` byte-for-byte before evaluating current +preconditions or running a step; it therefore preserves generated anchors, timestamps, versions, +outcome, semantic deltas, and `packageHash` from the original call. The result has one additional +top-level identity — not a parallel receipt model: + +```json +{ + "transaction": { + "schemaVersion": 1, + "transactionId": "caller-operation-42", + "requestFingerprint": "sha256:..." + } +} +``` + +The request fingerprint excludes only root `sessionId` and `transactionId`; it sorts object keys, +normalizes JSON whitespace and equivalent string escapes, preserves array order, string spelling, +numeric token spelling, unknown properties, and every omitted/explicit distinction except the root +default `mode` (`mode` omitted is canonicalized as `"atomic"`). Deprecated `apply` remains distinct +from `best_effort`. Duplicate keys are rejected at any depth. Reusing an id for a different +fingerprint returns `transaction_conflict`. The per-session journal retains 128 full responses, +then 1,024 response-less FIFO tombstones; an identical retry whose response has been evicted returns +`transaction_result_evicted`, while its tombstone still prevents conflicting reuse. Once the +tombstone expires—or the session is closed—the identity is no longer known. Transaction ids are +for mutating batches only: direct tools, step args, `mode: preview`, and `preview: true` reject them. +Replay after an ordinary undo or redo still returns the historical response: it never reapplies, +undoes, or redoes the mutation, changes the current document, or moves either history cursor. A +caller that wants an undone mutation present again must use ordinary `redo` while it remains +available. Saving preserves the in-session journal. Closing clears it, and reopening the document +starts a new transaction-identity namespace even when it opens the same saved file. + The batch itself and each step's `args` may carry `preconditions`, using the same camel-case guard object as the core API (`expectedVersion`, `anchorId`, `expectedContentHash`, exact text/range/kind/scope, and `expectedMatchCount`). A diff --git a/tools/mcp-server/README.md b/tools/mcp-server/README.md index e40edbc2..1d4630e1 100644 --- a/tools/mcp-server/README.md +++ b/tools/mcp-server/README.md @@ -97,6 +97,17 @@ markdown projection and search tools return: | `docxodus_mutations` | Apply or safely preview a batch atomically by default; opt explicitly into best-effort | | `docxodus_table` | Create/read tables; resolve canonical cell anchors ↔ grid coordinates; edit rows/columns/cell content/style | +Applying `docxodus_mutations` batches can include a caller-chosen root `transactionId`. During the +open session, retrying the same canonical request returns the exact original serialized batch +result without applying again or rechecking guards; a different request with that id fails with +`transaction_conflict`. Results expose +`transaction: { schemaVersion: 1, transactionId, requestFingerprint }`. Preview/dry-run batches and +direct or nested step calls reject transaction ids. Retention is bounded per session (128 complete +responses followed by 1,024 response-less tombstones). Save preserves this journal; close clears +it, and reopen starts a new identity namespace. Replay after undo/redo returns the historical +response without changing the document or either history cursor; use ordinary redo to restore an +undone mutation. + ## Known gaps A few capabilities a full-featured document-editing agent surface might want are not yet From ec6197d40a0f995c00d65a44668e08cd70a0a12d Mon Sep 17 00:00:00 2001 From: JSv4 Date: Fri, 14 Aug 2026 07:26:10 -0500 Subject: [PATCH 3/5] Harden mutation transaction audit invariants --- Docxodus.Tests/McpMutationTransactionTests.cs | 250 +++++++++++++++++- docs/architecture/docx_agent_server.md | 14 +- tools/mcp-server/Dispatcher.cs | 7 +- tools/mcp-server/MutationTransactions.cs | 25 +- tools/mcp-server/README.md | 7 +- tools/mcp-server/SessionStore.cs | 54 +++- tools/mcp-server/ToolCatalog.cs | 2 +- 7 files changed, 332 insertions(+), 27 deletions(-) diff --git a/Docxodus.Tests/McpMutationTransactionTests.cs b/Docxodus.Tests/McpMutationTransactionTests.cs index 07b43326..2960dc1d 100644 --- a/Docxodus.Tests/McpMutationTransactionTests.cs +++ b/Docxodus.Tests/McpMutationTransactionTests.cs @@ -8,6 +8,7 @@ namespace Docxodus.Tests; /// Issue #449: in-session idempotency and session-wide dispatch ordering. +[Collection("MCP session registry isolation")] public sealed class McpMutationTransactionTests : IDisposable { private readonly string _root; @@ -335,6 +336,100 @@ public void MCP449_JournalUsesGeneratedMetadataAndBoundedFullThenTombstoneFifos( journal.Begin("a", "sha256:fresh-after-both-fifos").Kind); } + [Fact] + public void MCP449_CompletionClockFailureAfterCommitStillReplaysWithoutApplyingAgain() + { + var now = new DateTimeOffset(2026, 8, 14, 13, 0, 0, TimeSpan.Zero); + var clockCalls = 0; + var journal = new MutationTransactions( + utcNow: () => ++clockCalls == 2 + ? throw new InvalidOperationException("completion clock failed") + : now, + recordIdFactory: () => "completion-record"); + using var store = new TestSessionStore( + new LocalFileDocumentStore(_root), () => journal); + var sessionId = OpenSession(store.Value, _path); + var args = MutationArgs( + sessionId, + "completion", + FirstAnchor(store.Value, sessionId), + "committed before clock failure"); + + var original = Dispatcher.Call(store.Value, "docxodus_mutations", J(args)); + var replay = Dispatcher.Call(store.Value, "docxodus_mutations", J(args)); + + Assert.True(J(original).GetProperty("success").GetBoolean()); + Assert.Equal(original, replay); + Assert.Equal(1, + Docxodus.Internal.DocxSessionOps.GetVersion(store.Value.Get(sessionId).Handle)); + Assert.Equal(1, Occurrences( + GetMarkdown(store.Value, sessionId), "committed before clock failure")); + var completed = Assert.IsType( + journal.GetRecord("completion")); + Assert.Equal(now, completed.StartedAt); + Assert.Equal(now, completed.CompletedAt); + Assert.Equal(original, completed.SerializedResponse); + Assert.Null(journal.GetTombstone("completion")); + } + + [Fact] + public void MCP449_EvictionClockFailureStillMovesTheExactIdentityToATombstone() + { + var start = new DateTimeOffset(2026, 8, 14, 14, 0, 0, TimeSpan.Zero); + var clockCalls = 0; + var recordNumber = 0; + DateTimeOffset Clock() + { + clockCalls++; + if (clockCalls == 5) throw new InvalidOperationException("eviction clock failed"); + return start.AddSeconds(clockCalls - 1); + } + var journal = new MutationTransactions( + fullRecordCapacity: 1, + tombstoneCapacity: 2, + utcNow: Clock, + recordIdFactory: () => $"eviction-record-{++recordNumber}"); + var first = AssertReserved(journal.Begin("first", "sha256:first")); + journal.Complete(first, "{\"result\":\"first exact\"}"); + var second = AssertReserved(journal.Begin("second", "sha256:second")); + + var completedSecond = journal.Complete(second, "{\"result\":\"second exact\"}"); + + Assert.Equal(1, journal.FullRecordCount); + Assert.Equal(1, journal.TombstoneCount); + Assert.Null(journal.GetRecord("first")); + var tombstone = Assert.IsType( + journal.GetTombstone("first")); + Assert.Equal(first.RecordId, tombstone.RecordId); + Assert.Equal(first.Identity, tombstone.Identity); + Assert.Equal(first.StartedAt, tombstone.StartedAt); + Assert.Equal(start.AddSeconds(1), tombstone.CompletedAt); + Assert.Equal(tombstone.CompletedAt, tombstone.EvictedAt); + Assert.Equal(MutationTransactionDecisionKind.ResultEvicted, + journal.Begin("first", "sha256:first").Kind); + Assert.Equal(MutationTransactionDecisionKind.Conflict, + journal.Begin("first", "sha256:different").Kind); + var replay = journal.Begin("second", "sha256:second"); + Assert.Equal(MutationTransactionDecisionKind.Replay, replay.Kind); + Assert.Same(completedSecond, replay.Record); + Assert.Equal("{\"result\":\"second exact\"}", replay.SerializedResponse); + } + + [Fact] + public void MCP449_AttachIdentitySerializesTheIdentitySchemaVersion() + { + var serialized = MutationTransactions.AttachIdentity( + "{\"success\":true}\n", + new MutationTransactionIdentity(7, "tx-version", "sha256:version")); + + Assert.EndsWith("\n", serialized, StringComparison.Ordinal); + var transaction = J(serialized).GetProperty("transaction"); + Assert.Equal(7, transaction.GetProperty("schemaVersion").GetInt32()); + Assert.Equal("tx-version", transaction.GetProperty("transactionId").GetString()); + Assert.Equal("sha256:version", + transaction.GetProperty("requestFingerprint").GetString()); + } + [Fact] public void MCP449_DispatcherSerializesEvictedResultAndConflictAndReusesOnlyAfterTombstone() { @@ -516,6 +611,151 @@ public void MCP449_SaveCloseAndCloseAllWaitForTheSameSessionDispatchGate() } } + [Fact] + public void MCP449_ReentrantDispatchAndLifecycleCallsFailBeforeLocksWhileExternalCloseWaits() + { + using var store = new TestSessionStore(new LocalFileDocumentStore(_root)); + var session = store.Value.Open( + DocxSession.CreateBlankDocxBytes(), null, new DocxSessionSettings()); + var otherSession = store.Value.Open( + DocxSession.CreateBlankDocxBytes(), null, new DocxSessionSettings()); + using var entered = new ManualResetEventSlim(false); + using var release = new ManualResetEventSlim(false); + var errors = new ConcurrentBag(); + var action = Task.Run(() => store.Value.Dispatch(session.Id, () => + { + errors.Add(Record.Exception(() => store.Value.Open( + DocxSession.CreateBlankDocxBytes(), null, new DocxSessionSettings()))); + errors.Add(Record.Exception(() => store.Value.Close(session.Id))); + errors.Add(Record.Exception(store.Value.CloseAll)); + errors.Add(Record.Exception(() => + store.Value.Dispatch(session.Id, () => "nested same session"))); + errors.Add(Record.Exception(() => + store.Value.Dispatch(otherSession.Id, () => "nested cross session"))); + entered.Set(); + release.Wait(); + return "{}"; + })); + Task? close = null; + try + { + Assert.True(entered.Wait(TimeSpan.FromSeconds(5))); + Assert.Equal(5, errors.Count); + Assert.All(errors, error => + { + var typed = Assert.IsType(error); + Assert.Contains("session dispatch callback", typed.Message, + StringComparison.Ordinal); + }); + close = Task.Run(() => store.Value.Close(session.Id)); + Assert.False(close.Wait(TimeSpan.FromMilliseconds(100))); + + release.Set(); + Assert.True(action.Wait(TimeSpan.FromSeconds(5))); + Assert.True(close.Wait(TimeSpan.FromSeconds(5))); + Assert.Throws(() => store.Value.Get(session.Id)); + } + finally + { + release.Set(); + action.Wait(TimeSpan.FromSeconds(5)); + close?.Wait(TimeSpan.FromSeconds(5)); + } + } + + [Fact] + public void MCP449_DifferentSessionsStillDispatchInParallel() + { + using var store = new TestSessionStore(new LocalFileDocumentStore(_root)); + var first = store.Value.Open( + DocxSession.CreateBlankDocxBytes(), null, new DocxSessionSettings()); + var second = store.Value.Open( + DocxSession.CreateBlankDocxBytes(), null, new DocxSessionSettings()); + using var firstEntered = new ManualResetEventSlim(false); + using var secondEntered = new ManualResetEventSlim(false); + using var release = new ManualResetEventSlim(false); + var firstAction = Task.Run(() => store.Value.Dispatch(first.Id, () => + { + firstEntered.Set(); + release.Wait(); + return "first"; + })); + var secondAction = Task.Run(() => store.Value.Dispatch(second.Id, () => + { + secondEntered.Set(); + release.Wait(); + return "second"; + })); + try + { + Assert.True(firstEntered.Wait(TimeSpan.FromSeconds(5))); + Assert.True(secondEntered.Wait(TimeSpan.FromSeconds(5))); + } + finally + { + release.Set(); + } + Assert.True(Task.WaitAll(new[] { firstAction, secondAction }, TimeSpan.FromSeconds(5))); + Assert.Equal("first", firstAction.Result); + Assert.Equal("second", secondAction.Result); + } + + [Fact] + public void MCP449_JournalFactoryFailureCannotLeakACoreSession() + { + var countBefore = Docxodus.Internal.SessionRegistry.Count; + using var store = new TestSessionStore( + new LocalFileDocumentStore(_root), + () => throw new InvalidOperationException("journal factory failed")); + + var error = Assert.Throws(() => store.Value.Open( + DocxSession.CreateBlankDocxBytes(), null, new DocxSessionSettings())); + + Assert.Equal("journal factory failed", error.Message); + Assert.Equal(countBefore, Docxodus.Internal.SessionRegistry.Count); + } + + [Fact] + public void MCP449_TransactionIdRuntimeUsesBlankAndUnicodeScalarContract() + { + var sessionId = OpenSession(_store, _path); + var anchor = FirstAnchor(_store, sessionId); + foreach (var invalid in new[] { "", " \t\r\n" }) + { + var error = Assert.Throws(() => Dispatcher.Call( + _store, + "docxodus_mutations", + J(MutationArgs(sessionId, invalid, anchor, "must not execute")))); + Assert.Contains("empty or whitespace", error.Message, StringComparison.Ordinal); + } + + var ascii256 = new string('a', 256); + var asciiResult = J(Dispatcher.Call(_store, "docxodus_mutations", + J(MutationArgs(sessionId, ascii256, anchor, "ascii boundary")))); + Assert.True(asciiResult.GetProperty("success").GetBoolean()); + Assert.NotNull(_store.Get(sessionId).MutationTransactions.GetRecord(ascii256)); + + var asciiError = Assert.Throws(() => Dispatcher.Call( + _store, + "docxodus_mutations", + J(MutationArgs(sessionId, new string('a', 257), anchor, "too long")))); + Assert.Contains("Unicode scalar values", asciiError.Message, StringComparison.Ordinal); + + var emoji256 = string.Concat(Enumerable.Repeat("\U0001F600", 256)); + Assert.Equal(512, emoji256.Length); + var emojiResult = J(Dispatcher.Call(_store, "docxodus_mutations", + J(MutationArgs(sessionId, emoji256, anchor, "emoji boundary")))); + Assert.True(emojiResult.GetProperty("success").GetBoolean()); + Assert.NotNull(_store.Get(sessionId).MutationTransactions.GetRecord(emoji256)); + + var emoji257 = emoji256 + "\U0001F600"; + var emojiError = Assert.Throws(() => Dispatcher.Call( + _store, + "docxodus_mutations", + J(MutationArgs(sessionId, emoji257, anchor, "too many emoji")))); + Assert.Contains("Unicode scalar values", emojiError.Message, StringComparison.Ordinal); + } + [Fact] public void MCP449_SaveThenRetryPreservesTheSavedMutationWithoutApplyingAgain() { @@ -548,8 +788,13 @@ public void MCP449_ToolSchemaDocumentsBoundedApplyingTransactionIdentity() Assert.Equal(1, transactionId.GetProperty("minLength").GetInt32()); Assert.Equal(MutationTransactions.MaxTransactionIdLength, transactionId.GetProperty("maxLength").GetInt32()); + Assert.Equal("\\S", transactionId.GetProperty("pattern").GetString()); Assert.Contains("APPLYING", transactionId.GetProperty("description").GetString(), StringComparison.Ordinal); + Assert.Contains("non-blank", transactionId.GetProperty("description").GetString(), + StringComparison.Ordinal); + Assert.Contains("Unicode scalar values", + transactionId.GetProperty("description").GetString(), StringComparison.Ordinal); Assert.Equal(128, MutationTransactions.DefaultFullRecordCapacity); Assert.Equal(1024, MutationTransactions.DefaultTombstoneCapacity); } @@ -667,7 +912,7 @@ private sealed class TestSessionStore : IDisposable { public TestSessionStore( IDocumentStore documents, - Func journalFactory) => + Func? journalFactory = null) => Value = new SessionStore(documents, journalFactory); public SessionStore Value { get; } @@ -675,3 +920,6 @@ public TestSessionStore( public void Dispose() => Value.CloseAll(); } } + +[CollectionDefinition("MCP session registry isolation", DisableParallelization = true)] +public sealed class McpSessionRegistryIsolationCollection; diff --git a/docs/architecture/docx_agent_server.md b/docs/architecture/docx_agent_server.md index b5b88e09..b62ac6ba 100644 --- a/docs/architecture/docx_agent_server.md +++ b/docs/architecture/docx_agent_server.md @@ -498,13 +498,13 @@ modulo those generated ids/timestamps. Such receipts carry warnings; clients mus id or `packageHash` equality unless the operation supplies stable ids/timestamps or is otherwise known deterministic. -Applying batches may carry a caller-chosen root `transactionId` (a non-empty string up to 256 -characters). Its first terminal success, partial result, structured failure, precondition failure, -or safely-caught exception is recorded for the lifetime of that open session. An identical retry -returns the original serialized `MutationBatchResult` byte-for-byte before evaluating current -preconditions or running a step; it therefore preserves generated anchors, timestamps, versions, -outcome, semantic deltas, and `packageHash` from the original call. The result has one additional -top-level identity — not a parallel receipt model: +Applying batches may carry a caller-chosen root `transactionId` (a non-blank string up to 256 +Unicode scalar values). Its first terminal success, partial result, structured failure, +precondition failure, or safely-caught exception is recorded for the lifetime of that open session. +An identical retry returns the original serialized `MutationBatchResult` byte-for-byte before +evaluating current preconditions or running a step; it therefore preserves generated anchors, +timestamps, versions, outcome, semantic deltas, and `packageHash` from the original call. The +result has one additional top-level identity — not a parallel receipt model: ```json { diff --git a/tools/mcp-server/Dispatcher.cs b/tools/mcp-server/Dispatcher.cs index 41fb6ed5..032c7080 100644 --- a/tools/mcp-server/Dispatcher.cs +++ b/tools/mcp-server/Dispatcher.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; using System.IO; +using System.Text; using System.Text.Json; using System.Text.RegularExpressions; using Docxodus; @@ -1023,9 +1024,11 @@ private static string ExecuteMutationRequest( var id = value.GetString()!; if (string.IsNullOrWhiteSpace(id)) throw new McpToolException("transactionId must not be empty or whitespace"); - if (id.Length > MutationTransactions.MaxTransactionIdLength) + var scalarLength = 0; + foreach (var _ in id.EnumerateRunes()) scalarLength++; + if (scalarLength > MutationTransactions.MaxTransactionIdLength) throw new McpToolException( - $"transactionId must not exceed {MutationTransactions.MaxTransactionIdLength} characters"); + $"transactionId must not exceed {MutationTransactions.MaxTransactionIdLength} Unicode scalar values"); return id; } diff --git a/tools/mcp-server/MutationTransactions.cs b/tools/mcp-server/MutationTransactions.cs index e5fc6479..550912ca 100644 --- a/tools/mcp-server/MutationTransactions.cs +++ b/tools/mcp-server/MutationTransactions.cs @@ -172,7 +172,10 @@ public MutationTransactionRecord Complete( var completed = current with { - CompletedAt = _utcNow(), + // The document mutation has already committed when Complete is called. Clock + // injection is diagnostic metadata and must never strand that committed result + // behind an active reservation that cannot replay. + CompletedAt = UtcNowOr(current.StartedAt), SerializedResponse = serializedResponse, }; _records[completed.Identity.TransactionId] = completed; @@ -196,7 +199,9 @@ private void EvictCompletedRecords() evicted.Identity, evicted.StartedAt, completedAt, - _utcNow()); + // Eviction must move the identity from a full record to a tombstone as one + // logical operation even when an injected diagnostics clock fails. + UtcNowOr(completedAt)); _tombstoneFifo.Enqueue(id); } @@ -207,6 +212,18 @@ private void EvictCompletedRecords() } } + private DateTimeOffset UtcNowOr(DateTimeOffset fallback) + { + try + { + return _utcNow(); + } + catch (Exception) + { + return fallback; + } + } + /// /// SHA-256 over a deterministic JSON rendering. Root session/transaction identity is excluded; /// objects are sorted, arrays and scalar spelling are retained, and numeric tokens are copied @@ -314,7 +331,9 @@ public static string AttachIdentity( var suffix = serializedBatchResult[(end + 1)..]; return serializedBatchResult[..end] - + ",\"transaction\":{\"schemaVersion\":1,\"transactionId\":" + + ",\"transaction\":{\"schemaVersion\":" + + identity.SchemaVersion.ToString(System.Globalization.CultureInfo.InvariantCulture) + + ",\"transactionId\":" + JsonRpcIo.JsonString(identity.TransactionId) + ",\"requestFingerprint\":" + JsonRpcIo.JsonString(identity.RequestFingerprint) diff --git a/tools/mcp-server/README.md b/tools/mcp-server/README.md index 1d4630e1..0885cebf 100644 --- a/tools/mcp-server/README.md +++ b/tools/mcp-server/README.md @@ -97,9 +97,10 @@ markdown projection and search tools return: | `docxodus_mutations` | Apply or safely preview a batch atomically by default; opt explicitly into best-effort | | `docxodus_table` | Create/read tables; resolve canonical cell anchors ↔ grid coordinates; edit rows/columns/cell content/style | -Applying `docxodus_mutations` batches can include a caller-chosen root `transactionId`. During the -open session, retrying the same canonical request returns the exact original serialized batch -result without applying again or rechecking guards; a different request with that id fails with +Applying `docxodus_mutations` batches can include a caller-chosen, non-blank root `transactionId` of +at most 256 Unicode scalar values. During the open session, retrying the same canonical request +returns the exact original serialized batch result without applying again or rechecking guards; a +different request with that id fails with `transaction_conflict`. Results expose `transaction: { schemaVersion: 1, transactionId, requestFingerprint }`. Preview/dry-run batches and direct or nested step calls reject transaction ids. Retention is bounded per session (128 complete diff --git a/tools/mcp-server/SessionStore.cs b/tools/mcp-server/SessionStore.cs index 13f178b5..9d8682b5 100644 --- a/tools/mcp-server/SessionStore.cs +++ b/tools/mcp-server/SessionStore.cs @@ -27,7 +27,7 @@ internal sealed class DocSession internal MutationTransactions MutationTransactions { get; init; } = new(); /// False after close has won the dispatch race for this session. - internal bool Active { get; set; } = true; + internal volatile bool Active = true; /// Store-resolved location this session was opened from — already checked to be in /// scope, so a save back to it needs no re-validation. Null only if a session was opened from @@ -45,6 +45,7 @@ internal sealed class SessionStore { private readonly ConcurrentDictionary _sessions = new(); private readonly object _lifecycleGate = new(); + private readonly AsyncLocal _dispatchDepth = new(); private readonly System.Func _mutationTransactionsFactory; /// Backing document store. Defaults to a local store rooted at the @@ -66,18 +67,30 @@ public SessionStore( public DocSession Open(byte[] bytes, string? location, DocxSessionSettings settings) { + RejectReentrantLifecycle("open"); + // Construct fallible per-session collaborators before allocating a core handle. If the + // factory fails, there is nothing in either registry to clean up. + var mutationTransactions = _mutationTransactionsFactory(); lock (_lifecycleGate) { var handle = DocxSessionOps.OpenSession(bytes, settings); - var session = new DocSession + try { - Id = NewSessionId(), - Handle = handle, - Location = location, - MutationTransactions = _mutationTransactionsFactory(), - }; - _sessions[session.Id] = session; - return session; + var session = new DocSession + { + Id = NewSessionId(), + Handle = handle, + Location = location, + MutationTransactions = mutationTransactions, + }; + _sessions[session.Id] = session; + return session; + } + catch + { + DocxSessionOps.CloseSession(handle); + throw; + } } } @@ -101,9 +114,13 @@ public DocSession Get(string sessionId) /// Run one complete session-bound dispatch while holding the session's synchronous gate. /// The active check after taking the gate closes the lookup/close race: a caller that found /// the session before close removed it still cannot enter the disposed core handle. + /// Callbacks are non-reentrant so lifecycle operations retain one lifecycle-to-session lock order. /// public string Dispatch(string sessionId, System.Func action) { + if (_dispatchDepth.Value > 0) + throw new McpToolException( + "cannot nest a session dispatch within a session dispatch callback"); if (!_sessions.TryGetValue(sessionId, out var session)) throw new McpToolException($"unknown session_id: {sessionId}"); lock (session.DispatchGate) @@ -112,12 +129,21 @@ public string Dispatch(string sessionId, System.Func action) || !_sessions.TryGetValue(sessionId, out var current) || !ReferenceEquals(session, current)) throw new McpToolException($"unknown session_id: {sessionId}"); - return action(); + _dispatchDepth.Value++; + try + { + return action(); + } + finally + { + _dispatchDepth.Value--; + } } } public void Close(string sessionId) { + RejectReentrantLifecycle("close"); lock (_lifecycleGate) { if (!_sessions.TryGetValue(sessionId, out var session)) return; @@ -133,6 +159,7 @@ public void Close(string sessionId) public void CloseAll() { + RejectReentrantLifecycle("close all sessions"); lock (_lifecycleGate) { foreach (var kv in _sessions) @@ -147,6 +174,13 @@ public void CloseAll() } } } + + private void RejectReentrantLifecycle(string operation) + { + if (_dispatchDepth.Value > 0) + throw new McpToolException( + $"cannot {operation} from within a session dispatch callback"); + } } /// Business-level tool failure — reported as an MCP tool result with isError: true, diff --git a/tools/mcp-server/ToolCatalog.cs b/tools/mcp-server/ToolCatalog.cs index 592d523e..b6e33244 100644 --- a/tools/mcp-server/ToolCatalog.cs +++ b/tools/mcp-server/ToolCatalog.cs @@ -508,7 +508,7 @@ internal static class ToolCatalog "type": "object", "properties": { "sessionId": { "type": "string" }, - "transactionId": { "type": "string", "minLength": 1, "maxLength": 256, "description": "Optional caller identity for an APPLYING batch only. The first terminal response is retained in this open session; an identical retry returns that exact serialized response without executing or rechecking preconditions. Reusing the id for a different canonical request returns transaction_conflict. Preview/dry-run rejects this field." }, + "transactionId": { "type": "string", "minLength": 1, "maxLength": 256, "pattern": "\\S", "description": "Optional non-blank caller identity for an APPLYING batch only, limited to 256 Unicode scalar values. The first terminal response is retained in this open session; an identical retry returns that exact serialized response without executing or rechecking preconditions. Reusing the id for a different canonical request returns transaction_conflict. Preview/dry-run rejects this field." }, "preconditions": { "type": "object", "description": "Optional batch-start guards. Each step args object may also carry its own preconditions." }, "mode": { "type": "string", "enum": ["atomic", "best_effort", "apply", "preview"], "default": "atomic", "description": "atomic (default): all steps commit as one undo/version unit or fully roll back. best_effort: explicitly retain successful steps after failures. apply: deprecated alias for best_effort. preview: isolated dry-run shorthand using atomic policy unless previewPolicy says best_effort." }, "preview": { "type": "boolean", "default": false, "description": "Dry-run mode for mode=atomic or mode=best_effort. The complete package is cloned and the live document, version, caches, configuration, and undo/redo history are never touched." }, From 9089c4478a6d2100656d05bc323ef3ac7d54a310 Mon Sep 17 00:00:00 2001 From: JSv4 Date: Fri, 14 Aug 2026 07:34:09 -0500 Subject: [PATCH 4/5] Align transaction whitespace and dispatch flow --- Docxodus.Tests/McpMutationTransactionTests.cs | 63 ++++++++++++++++++- tools/mcp-server/Dispatcher.cs | 2 +- tools/mcp-server/MutationTransactions.cs | 62 ++++++++++++++++++ tools/mcp-server/SessionStore.cs | 23 +++++-- tools/mcp-server/ToolCatalog.cs | 5 +- 5 files changed, 145 insertions(+), 10 deletions(-) diff --git a/Docxodus.Tests/McpMutationTransactionTests.cs b/Docxodus.Tests/McpMutationTransactionTests.cs index 2960dc1d..9d66bf5d 100644 --- a/Docxodus.Tests/McpMutationTransactionTests.cs +++ b/Docxodus.Tests/McpMutationTransactionTests.cs @@ -700,6 +700,53 @@ public void MCP449_DifferentSessionsStillDispatchInParallel() Assert.Equal("second", secondAction.Result); } + [Fact] + public void MCP449_FlowedChildDispatchRejectsWhileActiveButRunsAfterParentReturns() + { + using var parentStore = new TestSessionStore(new LocalFileDocumentStore(_root)); + using var childStore = new TestSessionStore(new LocalFileDocumentStore(_root)); + var parentSession = parentStore.Value.Open( + DocxSession.CreateBlankDocxBytes(), null, new DocxSessionSettings()); + var childSession = childStore.Value.Open( + DocxSession.CreateBlankDocxBytes(), null, new DocxSessionSettings()); + using var releaseDeferredChild = new ManualResetEventSlim(false); + Task? concurrentChild = null; + Task? deferredChild = null; + + var parent = Task.Run(() => parentStore.Value.Dispatch(parentSession.Id, () => + { + concurrentChild = Task.Run(() => Record.Exception(() => + childStore.Value.Dispatch(childSession.Id, () => "must reject"))); + if (!concurrentChild.Wait(TimeSpan.FromSeconds(5))) + throw new TimeoutException("concurrent child did not finish"); + + deferredChild = Task.Run(() => + { + releaseDeferredChild.Wait(); + return parentStore.Value.Dispatch(parentSession.Id, () => "deferred child"); + }); + return "parent"; + })); + + try + { + Assert.True(parent.Wait(TimeSpan.FromSeconds(5))); + Assert.Equal("parent", parent.Result); + var reentrant = Assert.IsType(concurrentChild!.Result); + Assert.Contains("session dispatch callback", reentrant.Message, + StringComparison.Ordinal); + + releaseDeferredChild.Set(); + Assert.True(deferredChild!.Wait(TimeSpan.FromSeconds(5))); + Assert.Equal("deferred child", deferredChild.Result); + } + finally + { + releaseDeferredChild.Set(); + deferredChild?.Wait(TimeSpan.FromSeconds(5)); + } + } + [Fact] public void MCP449_JournalFactoryFailureCannotLeakACoreSession() { @@ -720,7 +767,7 @@ public void MCP449_TransactionIdRuntimeUsesBlankAndUnicodeScalarContract() { var sessionId = OpenSession(_store, _path); var anchor = FirstAnchor(_store, sessionId); - foreach (var invalid in new[] { "", " \t\r\n" }) + foreach (var invalid in new[] { "", " \t\r\n", "\u0085" }) { var error = Assert.Throws(() => Dispatcher.Call( _store, @@ -729,6 +776,12 @@ public void MCP449_TransactionIdRuntimeUsesBlankAndUnicodeScalarContract() Assert.Contains("empty or whitespace", error.Message, StringComparison.Ordinal); } + const string byteOrderMark = "\uFEFF"; + var byteOrderMarkResult = J(Dispatcher.Call(_store, "docxodus_mutations", + J(MutationArgs(sessionId, byteOrderMark, anchor, "BOM is not whitespace")))); + Assert.True(byteOrderMarkResult.GetProperty("success").GetBoolean()); + Assert.NotNull(_store.Get(sessionId).MutationTransactions.GetRecord(byteOrderMark)); + var ascii256 = new string('a', 256); var asciiResult = J(Dispatcher.Call(_store, "docxodus_mutations", J(MutationArgs(sessionId, ascii256, anchor, "ascii boundary")))); @@ -788,13 +841,19 @@ public void MCP449_ToolSchemaDocumentsBoundedApplyingTransactionIdentity() Assert.Equal(1, transactionId.GetProperty("minLength").GetInt32()); Assert.Equal(MutationTransactions.MaxTransactionIdLength, transactionId.GetProperty("maxLength").GetInt32()); - Assert.Equal("\\S", transactionId.GetProperty("pattern").GetString()); + Assert.Equal( + @"[^\u0009-\u000D\u0020\u0085\u00A0\u1680\u2000-\u200A\u2028-\u2029\u202F\u205F\u3000]", + transactionId.GetProperty("pattern").GetString()); Assert.Contains("APPLYING", transactionId.GetProperty("description").GetString(), StringComparison.Ordinal); Assert.Contains("non-blank", transactionId.GetProperty("description").GetString(), StringComparison.Ordinal); Assert.Contains("Unicode scalar values", transactionId.GetProperty("description").GetString(), StringComparison.Ordinal); + Assert.Contains(MutationTransactions.TransactionIdWhiteSpaceDescription, + transactionId.GetProperty("description").GetString(), StringComparison.Ordinal); + Assert.Contains("U+FEFF is non-whitespace", + transactionId.GetProperty("description").GetString(), StringComparison.Ordinal); Assert.Equal(128, MutationTransactions.DefaultFullRecordCapacity); Assert.Equal(1024, MutationTransactions.DefaultTombstoneCapacity); } diff --git a/tools/mcp-server/Dispatcher.cs b/tools/mcp-server/Dispatcher.cs index 032c7080..2ae16101 100644 --- a/tools/mcp-server/Dispatcher.cs +++ b/tools/mcp-server/Dispatcher.cs @@ -1022,7 +1022,7 @@ private static string ExecuteMutationRequest( if (value.ValueKind != JsonValueKind.String) throw new McpToolException("transactionId must be a string"); var id = value.GetString()!; - if (string.IsNullOrWhiteSpace(id)) + if (MutationTransactions.IsBlankTransactionId(id)) throw new McpToolException("transactionId must not be empty or whitespace"); var scalarLength = 0; foreach (var _ in id.EnumerateRunes()) scalarLength++; diff --git a/tools/mcp-server/MutationTransactions.cs b/tools/mcp-server/MutationTransactions.cs index 550912ca..0e2cd4b5 100644 --- a/tools/mcp-server/MutationTransactions.cs +++ b/tools/mcp-server/MutationTransactions.cs @@ -61,6 +61,42 @@ internal sealed class MutationTransactions public const int DefaultTombstoneCapacity = 1024; public const int MaxTransactionIdLength = 256; + // Stable Unicode White_Space definition for transaction ids. Runtime validation, the schema + // regex, and its prose are all derived from this one table so their blank-string semantics + // cannot drift with .NET or ECMAScript whitespace classifications. + private static readonly (int Start, int End)[] TransactionIdWhiteSpaceRanges = + { + (0x0009, 0x000D), + (0x0020, 0x0020), + (0x0085, 0x0085), + (0x00A0, 0x00A0), + (0x1680, 0x1680), + (0x2000, 0x200A), + (0x2028, 0x2029), + (0x202F, 0x202F), + (0x205F, 0x205F), + (0x3000, 0x3000), + }; + + internal static string TransactionIdNonBlankPattern { get; } = + BuildTransactionIdNonBlankPattern(); + + internal static string TransactionIdWhiteSpaceDescription { get; } = + string.Join(", ", TransactionIdWhiteSpaceRanges.Select(static range => + range.Start == range.End + ? "U+" + ScalarHex(range.Start) + : "U+" + ScalarHex(range.Start) + "-U+" + ScalarHex(range.End))); + + internal static string TransactionIdSchemaDescription { get; } = + "Optional non-blank caller identity for an APPLYING batch only, limited to 256 Unicode " + + "scalar values. Blank means composed only of exactly these Unicode White_Space code " + + "points: " + + TransactionIdWhiteSpaceDescription + + "; U+FEFF is non-whitespace. The first terminal response is retained in this open " + + "session; an identical retry returns that exact serialized response without executing " + + "or rechecking preconditions. Reusing the id for a different canonical request returns " + + "transaction_conflict. Preview/dry-run rejects this field."; + private readonly int _fullRecordCapacity; private readonly int _tombstoneCapacity; private readonly Func _utcNow; @@ -110,6 +146,32 @@ internal int TombstoneCount return _tombstones.TryGetValue(transactionId, out var tombstone) ? tombstone : null; } + internal static bool IsBlankTransactionId(string transactionId) + { + foreach (var rune in transactionId.EnumerateRunes()) + { + if (!TransactionIdWhiteSpaceRanges.Any(range => + rune.Value >= range.Start && rune.Value <= range.End)) + return false; + } + return true; + } + + private static string BuildTransactionIdNonBlankPattern() + { + var pattern = new StringBuilder("[^"); + foreach (var range in TransactionIdWhiteSpaceRanges) + { + pattern.Append("\\u").Append(ScalarHex(range.Start)); + if (range.Start != range.End) + pattern.Append("-\\u").Append(ScalarHex(range.End)); + } + return pattern.Append(']').ToString(); + } + + private static string ScalarHex(int value) => + value.ToString("X4", System.Globalization.CultureInfo.InvariantCulture); + /// Reserve a new identity, or resolve it to replay/conflict/expired deterministically. public MutationTransactionDecision Begin(string transactionId, string requestFingerprint) { diff --git a/tools/mcp-server/SessionStore.cs b/tools/mcp-server/SessionStore.cs index 9d8682b5..d4a9c7be 100644 --- a/tools/mcp-server/SessionStore.cs +++ b/tools/mcp-server/SessionStore.cs @@ -43,9 +43,17 @@ internal sealed class DocSession /// internal sealed class SessionStore { + private sealed class DispatchFrame + { + public volatile bool Active = true; + } + + // Static scope rejects cross-store reentry too. ExecutionContext copies the frame reference, + // while Active is shared so deferred children cease being reentrant when the parent returns. + private static readonly AsyncLocal CurrentDispatch = new(); + private readonly ConcurrentDictionary _sessions = new(); private readonly object _lifecycleGate = new(); - private readonly AsyncLocal _dispatchDepth = new(); private readonly System.Func _mutationTransactionsFactory; /// Backing document store. Defaults to a local store rooted at the @@ -115,10 +123,12 @@ public DocSession Get(string sessionId) /// The active check after taking the gate closes the lookup/close race: a caller that found /// the session before close removed it still cannot enter the disposed core handle. /// Callbacks are non-reentrant so lifecycle operations retain one lifecycle-to-session lock order. + /// Callback-spawned work must preserve flow; + /// deliberately suppressed or unsafe flow is unsupported by this synchronous dispatch contract. /// public string Dispatch(string sessionId, System.Func action) { - if (_dispatchDepth.Value > 0) + if (CurrentDispatch.Value?.Active == true) throw new McpToolException( "cannot nest a session dispatch within a session dispatch callback"); if (!_sessions.TryGetValue(sessionId, out var session)) @@ -129,14 +139,17 @@ public string Dispatch(string sessionId, System.Func action) || !_sessions.TryGetValue(sessionId, out var current) || !ReferenceEquals(session, current)) throw new McpToolException($"unknown session_id: {sessionId}"); - _dispatchDepth.Value++; + var priorFrame = CurrentDispatch.Value; + var frame = new DispatchFrame(); + CurrentDispatch.Value = frame; try { return action(); } finally { - _dispatchDepth.Value--; + frame.Active = false; + CurrentDispatch.Value = priorFrame; } } } @@ -177,7 +190,7 @@ public void CloseAll() private void RejectReentrantLifecycle(string operation) { - if (_dispatchDepth.Value > 0) + if (CurrentDispatch.Value?.Active == true) throw new McpToolException( $"cannot {operation} from within a session dispatch callback"); } diff --git a/tools/mcp-server/ToolCatalog.cs b/tools/mcp-server/ToolCatalog.cs index b6e33244..714f3d99 100644 --- a/tools/mcp-server/ToolCatalog.cs +++ b/tools/mcp-server/ToolCatalog.cs @@ -1,6 +1,7 @@ #nullable enable using System.Collections.Generic; +using System.Text.Json; namespace Docxodus.McpServer; @@ -503,12 +504,12 @@ internal static class ToolCatalog new ToolDefinition( "docxodus_mutations", "Apply or safely preview a batch of mutating edit/format/create/table/list/comment/link/image/content-control/track-changes actions. Atomic mode commits as one unit. An optional transactionId makes applying retries idempotent within this open session; preview is isolated and cannot carry a transactionId.", - """ + $$""" { "type": "object", "properties": { "sessionId": { "type": "string" }, - "transactionId": { "type": "string", "minLength": 1, "maxLength": 256, "pattern": "\\S", "description": "Optional non-blank caller identity for an APPLYING batch only, limited to 256 Unicode scalar values. The first terminal response is retained in this open session; an identical retry returns that exact serialized response without executing or rechecking preconditions. Reusing the id for a different canonical request returns transaction_conflict. Preview/dry-run rejects this field." }, + "transactionId": { "type": "string", "minLength": 1, "maxLength": 256, "pattern": {{JsonSerializer.Serialize(MutationTransactions.TransactionIdNonBlankPattern)}}, "description": {{JsonSerializer.Serialize(MutationTransactions.TransactionIdSchemaDescription)}} }, "preconditions": { "type": "object", "description": "Optional batch-start guards. Each step args object may also carry its own preconditions." }, "mode": { "type": "string", "enum": ["atomic", "best_effort", "apply", "preview"], "default": "atomic", "description": "atomic (default): all steps commit as one undo/version unit or fully roll back. best_effort: explicitly retain successful steps after failures. apply: deprecated alias for best_effort. preview: isolated dry-run shorthand using atomic policy unless previewPolicy says best_effort." }, "preview": { "type": "boolean", "default": false, "description": "Dry-run mode for mode=atomic or mode=best_effort. The complete package is cloned and the live document, version, caches, configuration, and undo/redo history are never touched." }, From fa5e046188f9331af7c7e57a11223d9af115f70d Mon Sep 17 00:00:00 2001 From: JSv4 Date: Fri, 14 Aug 2026 22:14:14 -0500 Subject: [PATCH 5/5] fix(mcp): bound transaction retention by bytes and stop lying about incomplete ids MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up on the mutation transaction identity epic (#449). Retention was bounded by count only. A retained entry is a complete serialized MutationBatchResult, which emits every step's results twice plus patch.markdown and the semantic delta sets — a measured ~3.2 KB even for a one-step batch on a blank document — so 128 responses per session is not a memory bound. Adds a 32 MiB per-session byte budget alongside the count cap, evicting oldest-first until both hold. A response larger than the whole budget evicts itself rather than raising the ceiling; the identity stays bound and answers transaction_result_evicted. Byte accounting is decremented on every path a retained response leaves the record map. A reservation that never recorded a terminal response returned transaction_conflict quoting the RETRY's own fingerprint — the exact opposite of what happened — and could never be reclaimed. It now has its own decision kind and EditErrorCode.TransactionIncomplete ("outcome unknown"), the Dispatcher abandons any reservation it cannot complete, an abandoned reservation becomes an outcome-unknown tombstone, and uncompleted reservations are FIFO-bounded like completed ones. Also: - CHANGELOG [Unreleased] entry covering the new transactionId field, all four error codes, and the retention bound. - HttpTransport's doc comment said SessionStore assumes single-threaded access, which this epic made false. Corrected to state that per-session dispatch is forward-looking: Handle runs inline on the serial accept loop, so the lock is not what serializes requests, and making concurrency real means moving handling off the accept loop AND dropping the lock — a deliberate behaviour change, not a comment fix. Notes the same for the lifecycle gate around open. - ToolCatalog and the schema description interpolate MaxTransactionIdLength instead of repeating 256. - Document the retention cost, the validation-failure-burns-the-id rule, the tombstone-expiry re-apply hazard, and that idempotency is MCP-only. Tests: five new MCP449 facts (byte-budget eviction incl. the oversized-response boundary, measured real retained cost, incomplete-vs-conflict truthfulness and evictability, reservation FIFO bound, Dispatcher mapping), plus wire-string parity assertions on both client surfaces. --- CHANGELOG.md | 26 +++ Docxodus.Tests/McpMutationTransactionTests.cs | 120 ++++++++++++++ Docxodus/DocxSession.cs | 3 + docs/architecture/docx_agent_server.md | 46 +++++- npm/src/types.ts | 1 + npm/tests/transaction-error-codes.spec.ts | 27 +++ python/src/docx_scalpel/enums.py | 1 + python/tests/test_transaction_error_codes.py | 40 +++++ tools/mcp-server/Dispatcher.cs | 59 +++++-- tools/mcp-server/HttpTransport.cs | 9 +- tools/mcp-server/MutationTransactions.cs | 156 +++++++++++++++--- tools/mcp-server/README.md | 44 ++++- tools/mcp-server/SessionStore.cs | 4 + tools/mcp-server/ToolCatalog.cs | 2 +- 14 files changed, 483 insertions(+), 55 deletions(-) create mode 100644 npm/tests/transaction-error-codes.spec.ts create mode 100644 python/tests/test_transaction_error_codes.py diff --git a/CHANGELOG.md b/CHANGELOG.md index f85ad01f..b53296ee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,32 @@ All notable changes to this project will be documented in this file. ## [Unreleased] ### Added +- **Idempotent mutation transaction identities for the MCP server** (issue #449). + An applying `docxodus_mutations` batch may carry a caller-chosen root + `transactionId` (non-blank, at most 256 Unicode scalar values). The first + terminal response — success, partial, structured failure, precondition failure, + or safely-caught exception — is retained for the lifetime of that open session, + and an identical retry returns it byte-for-byte without executing anything or + re-evaluating preconditions, so generated anchors, timestamps, versions and + `packageHash` all survive a lost response. Results gain a top-level + `transaction: { schemaVersion, transactionId, requestFingerprint }`; the + fingerprint is a SHA-256 over a canonical rendering that excludes only the root + `sessionId`/`transactionId`. Reusing an id for a different request returns + `transaction_conflict`. Preview/dry-run batches, nested step args, and the other + tools reject transaction ids rather than ignoring them. Retention is bounded per + session by both a count and a byte budget (128 responses, 32 MiB) followed by + 1,024 response-less tombstones; there is no TTL and the number of open sessions + is not bounded, and once a tombstone expires a late retry applies again — both + documented as hazards in + [`docs/architecture/docx_agent_server.md`](docs/architecture/docx_agent_server.md). + Idempotency is MCP-only: `execute_batch` through WASM/npm and the stdio host has + no equivalent. Adds `EditErrorCode.InvalidTransaction`, `TransactionConflict`, + `TransactionResultEvicted` and `TransactionIncomplete`, rippled to npm + `EditErrorCode` and Python `EditErrorCode`. MCP session dispatch is now + serialized per session so a retry cannot race another action on the same + document. Coverage: `McpMutationTransactionTests` MCP449, + `python/tests/test_transaction_error_codes.py`, and + `npm/tests/transaction-error-codes.spec.ts`. - **Canonical table addressing and complete table-operation ripple (#450, absorbing #471).** Tables now expose explicit stable identities for the `w:tbl`, every `w:tr`, every physical `w:tc`, and every `w:tblGrid/w:gridCol`, plus diff --git a/Docxodus.Tests/McpMutationTransactionTests.cs b/Docxodus.Tests/McpMutationTransactionTests.cs index 9d66bf5d..71b1446c 100644 --- a/Docxodus.Tests/McpMutationTransactionTests.cs +++ b/Docxodus.Tests/McpMutationTransactionTests.cs @@ -856,6 +856,126 @@ public void MCP449_ToolSchemaDocumentsBoundedApplyingTransactionIdentity() transactionId.GetProperty("description").GetString(), StringComparison.Ordinal); Assert.Equal(128, MutationTransactions.DefaultFullRecordCapacity); Assert.Equal(1024, MutationTransactions.DefaultTombstoneCapacity); + Assert.Equal(32L * 1024 * 1024, MutationTransactions.DefaultResponseByteBudget); + } + + [Fact] + public void MCP449_RetainedResponsesAreBoundedByBytesNotOnlyByCount() + { + // The count cap is deliberately generous here so only the byte budget can evict. + var journal = new MutationTransactions( + fullRecordCapacity: 64, tombstoneCapacity: 8, responseByteBudget: 200); + string Response(char fill) => "{\"r\":\"" + new string(fill, 40) + "\"}"; + Assert.Equal(96, (long)Response('a').Length * sizeof(char)); + + journal.Complete(AssertReserved(journal.Begin("a", "sha256:a")), Response('a')); + journal.Complete(AssertReserved(journal.Begin("b", "sha256:b")), Response('b')); + Assert.Equal(192, journal.RetainedResponseBytes); + Assert.Equal(2, journal.FullRecordCount); + Assert.Equal(0, journal.TombstoneCount); + + // 288 bytes would exceed the 200-byte budget, so the oldest retained response goes even + // though the count cap is nowhere near reached. + journal.Complete(AssertReserved(journal.Begin("c", "sha256:c")), Response('c')); + Assert.Equal(192, journal.RetainedResponseBytes); + Assert.Equal(2, journal.FullRecordCount); + Assert.Equal(1, journal.TombstoneCount); + Assert.Null(journal.GetRecord("a")); + Assert.Equal(MutationTransactionDecisionKind.ResultEvicted, + journal.Begin("a", "sha256:a").Kind); + Assert.Equal(MutationTransactionDecisionKind.Replay, journal.Begin("c", "sha256:c").Kind); + + // A single response larger than the whole budget evicts itself rather than raising the + // ceiling: the identity stays bound and answers "evicted", and the bound stays a bound. + journal.Complete( + AssertReserved(journal.Begin("huge", "sha256:huge")), new string('x', 200)); + Assert.Equal(0, journal.RetainedResponseBytes); + Assert.Equal(0, journal.FullRecordCount); + Assert.Equal(MutationTransactionDecisionKind.ResultEvicted, + journal.Begin("huge", "sha256:huge").Kind); + } + + [Fact] + public void MCP449_RetainedResponseCostIsMeasuredFromRealBatchResults() + { + var sessionId = OpenSession(_store, _path); + var journal = _store.Get(sessionId).MutationTransactions; + var response = Dispatcher.Call(_store, "docxodus_mutations", J(MutationArgs( + sessionId, "tx-cost", FirstAnchor(_store, sessionId), "one small step"))); + + Assert.Equal((long)response.Length * sizeof(char), journal.RetainedResponseBytes); + // Anchors the per-session memory cost documented in tools/mcp-server/README.md: even a + // one-step batch retains kilobytes, because a MutationBatchResult carries every step's + // results twice plus the markdown patch and the semantic delta sets. + Assert.InRange(journal.RetainedResponseBytes, 1_024L, 128L * 1024); + } + + [Fact] + public void MCP449_IncompleteReservationIsReportedTruthfullyAndIsEvictable() + { + var journal = new MutationTransactions(fullRecordCapacity: 4, tombstoneCapacity: 4); + var reservation = AssertReserved(journal.Begin("stranded", "sha256:original")); + + // The fingerprints match, so reporting a conflict would state the opposite of the truth. + var identical = journal.Begin("stranded", "sha256:original"); + Assert.Equal(MutationTransactionDecisionKind.Incomplete, identical.Kind); + Assert.Equal("sha256:original", identical.ExistingIdentity!.RequestFingerprint); + + // A genuinely different request still conflicts, against the ORIGINAL fingerprint. + var different = journal.Begin("stranded", "sha256:other"); + Assert.Equal(MutationTransactionDecisionKind.Conflict, different.Kind); + Assert.Equal("sha256:original", different.ExistingIdentity!.RequestFingerprint); + + journal.Abandon(reservation); + Assert.Null(journal.GetRecord("stranded")); + var tombstone = Assert.IsType( + journal.GetTombstone("stranded")); + Assert.Null(tombstone.CompletedAt); + Assert.Equal(MutationTransactionDecisionKind.Incomplete, + journal.Begin("stranded", "sha256:original").Kind); + Assert.Equal(MutationTransactionDecisionKind.Conflict, + journal.Begin("stranded", "sha256:other").Kind); + + journal.Abandon(reservation); // idempotent — no second tombstone, no resurrection + Assert.Equal(1, journal.TombstoneCount); + Assert.Throws(() => journal.Complete(reservation, "{}")); + } + + [Fact] + public void MCP449_UncompletedReservationsAreBoundedByTheirOwnFifo() + { + var journal = new MutationTransactions(fullRecordCapacity: 2, tombstoneCapacity: 8); + AssertReserved(journal.Begin("r1", "sha256:r1")); + AssertReserved(journal.Begin("r2", "sha256:r2")); + AssertReserved(journal.Begin("r3", "sha256:r3")); + + Assert.Null(journal.GetRecord("r1")); + Assert.Null(Assert.IsType( + journal.GetTombstone("r1")).CompletedAt); + Assert.Equal(MutationTransactionDecisionKind.Incomplete, + journal.Begin("r1", "sha256:r1").Kind); + Assert.NotNull(journal.GetRecord("r2")); + Assert.NotNull(journal.GetRecord("r3")); + } + + [Fact] + public void MCP449_DispatcherReportsAnIncompleteTransactionInsteadOfALyingConflict() + { + var sessionId = OpenSession(_store, _path); + var args = MutationArgs( + sessionId, "tx-stranded", FirstAnchor(_store, sessionId), "must not execute"); + + // Strand a reservation the way only a direct component caller can, then retry identically. + AssertReserved(_store.Get(sessionId).MutationTransactions.Begin( + "tx-stranded", MutationTransactions.Fingerprint(J(args)))); + + var error = J(Dispatcher.Call(_store, "docxodus_mutations", J(args))) + .GetProperty("failure").GetProperty("error"); + Assert.Equal("transaction_incomplete", error.GetProperty("code").GetString()); + Assert.DoesNotContain("different request fingerprint", + error.GetProperty("message").GetString()!, StringComparison.Ordinal); + Assert.Equal(0, Docxodus.Internal.DocxSessionOps.GetVersion(_store.Get(sessionId).Handle)); + Assert.Equal(0, Occurrences(GetMarkdown(_store, sessionId), "must not execute")); } private static MutationTransactionRecord AssertReserved(MutationTransactionDecision decision) diff --git a/Docxodus/DocxSession.cs b/Docxodus/DocxSession.cs index bd3d7f47..fefa228e 100644 --- a/Docxodus/DocxSession.cs +++ b/Docxodus/DocxSession.cs @@ -1706,6 +1706,9 @@ public enum EditErrorCode /// The exact response for a known transaction was evicted from bounded retention. TransactionResultEvicted, + /// A known transaction never recorded a terminal response, so its outcome is unknown. + TransactionIncomplete, + /// The revision family is visible but has no safe selective resolver. RevisionUnsupported, diff --git a/docs/architecture/docx_agent_server.md b/docs/architecture/docx_agent_server.md index b62ac6ba..fa0d3ed9 100644 --- a/docs/architecture/docx_agent_server.md +++ b/docs/architecture/docx_agent_server.md @@ -521,17 +521,53 @@ normalizes JSON whitespace and equivalent string escapes, preserves array order, numeric token spelling, unknown properties, and every omitted/explicit distinction except the root default `mode` (`mode` omitted is canonicalized as `"atomic"`). Deprecated `apply` remains distinct from `best_effort`. Duplicate keys are rejected at any depth. Reusing an id for a different -fingerprint returns `transaction_conflict`. The per-session journal retains 128 full responses, -then 1,024 response-less FIFO tombstones; an identical retry whose response has been evicted returns -`transaction_result_evicted`, while its tombstone still prevents conflicting reuse. Once the -tombstone expires—or the session is closed—the identity is no longer known. Transaction ids are -for mutating batches only: direct tools, step args, `mode: preview`, and `preview: true` reject them. +fingerprint returns `transaction_conflict`. Transaction ids are for mutating batches only: direct +tools, step args, `mode: preview`, and `preview: true` reject them. Replay after an ordinary undo or redo still returns the historical response: it never reapplies, undoes, or redoes the mutation, changes the current document, or moves either history cursor. A caller that wants an undone mutation present again must use ordinary `redo` while it remains available. Saving preserves the in-session journal. Closing clears it, and reopening the document starts a new transaction-identity namespace even when it opens the same saved file. +**Retention is bounded by count *and* bytes.** The per-session journal retains at most 128 full +responses *and* at most 32 MiB of retained response text (`DefaultFullRecordCapacity` / +`DefaultResponseByteBudget`), evicting oldest-first until both hold, then keeps 1,024 response-less +FIFO tombstones. A count alone is not a memory bound: a retained entry is the complete serialized +`MutationBatchResult`, which emits every step's `results` twice (`steps[].results` plus the +duplicate top-level `results`, `DocxSessionJson.cs`) alongside `patch.markdown` and the +revision/comment/annotation deltas. A one-step `insert_paragraph` batch on a blank document already +measures ~3.2 KB retained; scoped batches over real documents are far larger, so the worst case is +~32 MiB per open session. A single response exceeding the whole budget evicts itself rather than +raising the ceiling — the identity stays bound and answers `transaction_result_evicted`, which is a +safe answer, where an unbounded retained response would not be a bound at all. **Not bounded:** +the number of open sessions, and elapsed time — there is no TTL and no idle-session eviction, so +sessions that are never closed accumulate. A TTL / idle-session reaper is deliberately left as a +separate design question rather than smuggled in here. + +An identical retry whose response has been evicted returns `transaction_result_evicted`, while its +tombstone still prevents conflicting reuse. An id whose reservation never recorded a terminal +response returns `transaction_incomplete`: the fingerprints match, so reporting a conflict would +state the opposite of the truth, and the caller's real situation is that the outcome is unknown. +The Dispatcher cannot strand a reservation — it completes or abandons every one — so this is +reachable through direct component use; an abandoned reservation becomes an outcome-unknown +tombstone, and uncompleted reservations are FIFO-bounded like completed ones. + +Three lifecycle facts a client must design around: + +- **A validation failure burns the id.** `mode: "sideways"` is a terminal structured response and is + cached as such, so correcting the typo and resending under the same id yields + `transaction_conflict`. Use a fresh id after any failure you intend to correct; reuse an id only + to resend a byte-identical request. +- **HAZARD — tombstone expiry lets a stale retry silently re-apply.** Once ~128 further + transactions plus 1,024 further tombstones have passed, the identity is genuinely forgotten and a + late retry executes as a *fresh mutation* (`MCP449_DispatcherSerializesEvictedResultAndConflictAndReusesOnlyAfterTombstone` + pins exactly this: version 2 → 4). Idempotency here is a bounded-window guarantee, not a + permanent one. +- **The guarantee is MCP-only.** `execute_batch` via WASM/npm and via the stdio host / + `docx-scalpel` carries no transaction identity and no replay; a retry on those transports + re-applies. This asymmetry is within issue #449's scope, but callers must not generalize the + guarantee across transports. + The batch itself and each step's `args` may carry `preconditions`, using the same camel-case guard object as the core API (`expectedVersion`, `anchorId`, `expectedContentHash`, exact text/range/kind/scope, and `expectedMatchCount`). A diff --git a/npm/src/types.ts b/npm/src/types.ts index 094d7e96..e15b52ff 100644 --- a/npm/src/types.ts +++ b/npm/src/types.ts @@ -1357,6 +1357,7 @@ export type EditErrorCode = | "invalid_transaction" | "transaction_conflict" | "transaction_result_evicted" + | "transaction_incomplete" | "hyperlink_not_found" | "bookmark_not_found" | "duplicate_bookmark_name" diff --git a/npm/tests/transaction-error-codes.spec.ts b/npm/tests/transaction-error-codes.spec.ts new file mode 100644 index 00000000..9cef52cb --- /dev/null +++ b/npm/tests/transaction-error-codes.spec.ts @@ -0,0 +1,27 @@ +import { test, expect } from '@playwright/test'; +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import type { EditErrorCode } from '../src/types.js'; + +// The MCP server produces these codes; the browser package has no transaction surface of its +// own (idempotent retries are MCP-only). It does type every EditError it decodes, so the union +// has to name them. Declaring the list as EditErrorCode[] makes `npm run typecheck` fail if a +// member is dropped or misspelled, and the source assertion below catches it at runtime too. +const TRANSACTION_ERROR_CODES: readonly EditErrorCode[] = [ + 'invalid_transaction', + 'transaction_conflict', + 'transaction_result_evicted', + 'transaction_incomplete', +]; + +const typesSource = readFileSync( + fileURLToPath(new URL('../src/types.ts', import.meta.url)), + 'utf8', +); + +test('EditErrorCode names every MCP mutation-transaction wire string', () => { + for (const code of TRANSACTION_ERROR_CODES) { + expect(typesSource).toContain(`| "${code}"`); + } + expect(new Set(TRANSACTION_ERROR_CODES).size).toBe(TRANSACTION_ERROR_CODES.length); +}); diff --git a/python/src/docx_scalpel/enums.py b/python/src/docx_scalpel/enums.py index 39bf8adb..51b3d969 100644 --- a/python/src/docx_scalpel/enums.py +++ b/python/src/docx_scalpel/enums.py @@ -182,6 +182,7 @@ class EditErrorCode(str, Enum): INVALID_TRANSACTION = "invalid_transaction" TRANSACTION_CONFLICT = "transaction_conflict" TRANSACTION_RESULT_EVICTED = "transaction_result_evicted" + TRANSACTION_INCOMPLETE = "transaction_incomplete" HYPERLINK_NOT_FOUND = "hyperlink_not_found" BOOKMARK_NOT_FOUND = "bookmark_not_found" DUPLICATE_BOOKMARK_NAME = "duplicate_bookmark_name" diff --git a/python/tests/test_transaction_error_codes.py b/python/tests/test_transaction_error_codes.py new file mode 100644 index 00000000..48bf8f9f --- /dev/null +++ b/python/tests/test_transaction_error_codes.py @@ -0,0 +1,40 @@ +"""Wire-string parity for the MCP mutation-transaction ``EditErrorCode`` members. + +These codes are produced by ``tools/mcp-server`` and generated from the C# enum by +``EnumToSnake``. ``docx-scalpel`` itself has no transaction surface (idempotent retries +are MCP-only), but it decodes every ``EditError`` on the wire, so a client that talks to +the MCP server through another path must still be able to name them. +""" + +from __future__ import annotations + +from pathlib import Path + +from docx_scalpel.enums import EditErrorCode + +TRANSACTION_CODES = { + "invalid_transaction": "INVALID_TRANSACTION", + "transaction_conflict": "TRANSACTION_CONFLICT", + "transaction_result_evicted": "TRANSACTION_RESULT_EVICTED", + "transaction_incomplete": "TRANSACTION_INCOMPLETE", +} + + +def test_transaction_error_codes_round_trip_from_their_wire_strings() -> None: + for wire, member_name in TRANSACTION_CODES.items(): + member = getattr(EditErrorCode, member_name) + assert member.value == wire + # ``_missing_`` degrades an unknown code to INTERNAL_ERROR, so a member that was + # never added would decode silently. Decoding the wire string and demanding the + # exact member is what actually catches an absent or drifted code. + assert EditErrorCode(wire) is member + + +def test_transaction_error_codes_are_declared_in_this_checkout() -> None: + # Guards against an unrelated installed copy of docx_scalpel satisfying the import + # above: the source of record for this repository must declare them too. + source = ( + Path(__file__).resolve().parents[1] / "src" / "docx_scalpel" / "enums.py" + ).read_text(encoding="utf-8") + for wire, member_name in TRANSACTION_CODES.items(): + assert f'{member_name} = "{wire}"' in source diff --git a/tools/mcp-server/Dispatcher.cs b/tools/mcp-server/Dispatcher.cs index 2ae16101..f09f4ab8 100644 --- a/tools/mcp-server/Dispatcher.cs +++ b/tools/mcp-server/Dispatcher.cs @@ -897,6 +897,19 @@ private static string Mutations(SessionStore store, JsonElement args) "transaction"); return MutationTransactions.AttachIdentity(expired, identity); } + case MutationTransactionDecisionKind.Incomplete: + { + var incomplete = MutationTransactions.SerializeFailure( + RequestedCoreMode(args), + preview: false, + SafeVersion(liveSession), + EditErrorCode.TransactionIncomplete, + "this transactionId is bound to this exact request but never recorded a " + + "terminal response, so whether the mutation applied is unknown; inspect the " + + "document and retry under a new transactionId", + "transaction"); + return MutationTransactions.AttachIdentity(incomplete, identity); + } case MutationTransactionDecisionKind.Reserved: break; default: @@ -904,28 +917,40 @@ private static string Mutations(SessionStore store, JsonElement args) } var reservation = decision.Record!; - string terminalResponse; + var completed = false; try { - terminalResponse = MutationTransactions.AttachIdentity( - ExecuteMutationRequest(liveSession, args, transactional: true), identity); + string terminalResponse; + try + { + terminalResponse = MutationTransactions.AttachIdentity( + ExecuteMutationRequest(liveSession, args, transactional: true), identity); + } + catch (Exception ex) + { + var callerError = ex is McpToolException + or FormatException or JsonException or OverflowException; + terminalResponse = MutationTransactions.AttachIdentity( + MutationTransactions.SerializeFailure( + RequestedCoreMode(args), + preview: false, + SafeVersion(liveSession), + callerError ? EditErrorCode.InvalidBatchStep : EditErrorCode.InternalError, + ex.Message, + callerError ? "validation" : "dispatch"), + identity); + } + liveSession.MutationTransactions.Complete(reservation, terminalResponse); + completed = true; + return terminalResponse; } - catch (Exception ex) + finally { - var callerError = ex is McpToolException - or FormatException or JsonException or OverflowException; - terminalResponse = MutationTransactions.AttachIdentity( - MutationTransactions.SerializeFailure( - RequestedCoreMode(args), - preview: false, - SafeVersion(liveSession), - callerError ? EditErrorCode.InvalidBatchStep : EditErrorCode.InternalError, - ex.Message, - callerError ? "validation" : "dispatch"), - identity); + // If serializing the failure or retaining the response itself threw, the reservation + // would otherwise be stranded in the journal forever. Retire it to an outcome-unknown + // tombstone so the identity stays bound, stays truthful, and stays evictable. + if (!completed) liveSession.MutationTransactions.Abandon(reservation); } - liveSession.MutationTransactions.Complete(reservation, terminalResponse); - return terminalResponse; } private static string ExecuteMutationRequest( diff --git a/tools/mcp-server/HttpTransport.cs b/tools/mcp-server/HttpTransport.cs index 3c953924..e92d3118 100644 --- a/tools/mcp-server/HttpTransport.cs +++ b/tools/mcp-server/HttpTransport.cs @@ -13,8 +13,13 @@ namespace Docxodus.McpServer; /// streamable-HTTP binding allows a server to choose. No SSE, no session-id handshake, no TLS: /// this exists so the stdio server can be put behind a tunnel (e.g. ngrok http PORT) and /// pointed at from a ChatGPT Apps / remote-MCP developer setup, which cannot spawn a local -/// process. Requests are processed one at a time under a lock — and -/// the Docxodus session registry assume single-threaded access, exactly as stdio provides. +/// process. Requests are processed one at a time: handles each request inline +/// on the serial accept loop, and the lock below pins that even if handling ever moves off it. +/// does now serialize per session rather than requiring a +/// single-threaded caller, but nothing in the shipped server exercises that: this transport is +/// serial and stdio is single-threaded, so the per-session dispatch gate is forward-looking. +/// Making concurrent requests real means dispatching handling off the accept loop AND dropping +/// this lock — a deliberate behaviour change, not a comment fix. /// internal static class HttpTransport { diff --git a/tools/mcp-server/MutationTransactions.cs b/tools/mcp-server/MutationTransactions.cs index 0e2cd4b5..b23a9481 100644 --- a/tools/mcp-server/MutationTransactions.cs +++ b/tools/mcp-server/MutationTransactions.cs @@ -28,11 +28,16 @@ internal sealed record MutationTransactionRecord( DateTimeOffset? CompletedAt, string? SerializedResponse); +/// +/// An identity that is still bound but whose response is gone. is the +/// discriminator: a value means a terminal response existed and was evicted; null means the +/// reservation never recorded one, so the mutation's outcome is unknown. +/// internal sealed record MutationTransactionTombstone( string RecordId, MutationTransactionIdentity Identity, DateTimeOffset StartedAt, - DateTimeOffset CompletedAt, + DateTimeOffset? CompletedAt, DateTimeOffset EvictedAt); internal enum MutationTransactionDecisionKind @@ -41,6 +46,10 @@ internal enum MutationTransactionDecisionKind Replay, Conflict, ResultEvicted, + + /// The id is bound to this exact request, but no terminal response was ever + /// recorded for it, so whether the mutation applied is unknown. + Incomplete, } internal sealed record MutationTransactionDecision( @@ -53,6 +62,8 @@ internal sealed record MutationTransactionDecision( /// Bounded, per-session transaction-id registry. Full responses and response-less tombstones use /// independent FIFO limits. A tombstone keeps an evicted id bound to its original fingerprint for /// a further window, preventing a recently forgotten retry from becoming a fresh mutation. +/// Retained responses are additionally bounded by a byte budget, because a batch result is +/// unbounded in size while a count is not a memory bound. /// internal sealed class MutationTransactions { @@ -61,6 +72,14 @@ internal sealed class MutationTransactions public const int DefaultTombstoneCapacity = 1024; public const int MaxTransactionIdLength = 256; + /// + /// Ceiling on the UTF-16 payload of all retained responses for one session. Chosen so the + /// 128-response count cap stays the binding constraint for ordinary batches and this budget + /// only bites on unusually large ones; see tools/mcp-server/README.md for the measured + /// per-response cost this is sized against. + /// + public const long DefaultResponseByteBudget = 32L * 1024 * 1024; + // Stable Unicode White_Space definition for transaction ids. Runtime validation, the schema // regex, and its prose are all derived from this one table so their blank-string semantics // cannot drift with .NET or ECMAScript whitespace classifications. @@ -88,7 +107,9 @@ private static readonly (int Start, int End)[] TransactionIdWhiteSpaceRanges = : "U+" + ScalarHex(range.Start) + "-U+" + ScalarHex(range.End))); internal static string TransactionIdSchemaDescription { get; } = - "Optional non-blank caller identity for an APPLYING batch only, limited to 256 Unicode " + "Optional non-blank caller identity for an APPLYING batch only, limited to " + + MaxTransactionIdLength.ToString(System.Globalization.CultureInfo.InvariantCulture) + + " Unicode " + "scalar values. Blank means composed only of exactly these Unicode White_Space code " + "points: " + TransactionIdWhiteSpaceDescription @@ -99,6 +120,7 @@ private static readonly (int Start, int End)[] TransactionIdWhiteSpaceRanges = private readonly int _fullRecordCapacity; private readonly int _tombstoneCapacity; + private readonly long _responseByteBudget; private readonly Func _utcNow; private readonly Func _recordIdFactory; private readonly Dictionary _records = @@ -108,18 +130,27 @@ private static readonly (int Start, int End)[] TransactionIdWhiteSpaceRanges = new(StringComparer.Ordinal); private readonly Queue _tombstoneFifo = new(); + // Reservations are tracked by reference, not id: an id can be reserved again after its + // tombstone expires, and a stale queue entry must never prune that later live reservation. + private readonly Queue _reservationFifo = new(); + private long _retainedResponseBytes; + public MutationTransactions( int fullRecordCapacity = DefaultFullRecordCapacity, int tombstoneCapacity = DefaultTombstoneCapacity, Func? utcNow = null, - Func? recordIdFactory = null) + Func? recordIdFactory = null, + long responseByteBudget = DefaultResponseByteBudget) { if (fullRecordCapacity < 1) throw new ArgumentOutOfRangeException(nameof(fullRecordCapacity)); if (tombstoneCapacity < 0) throw new ArgumentOutOfRangeException(nameof(tombstoneCapacity)); + if (responseByteBudget < 1) + throw new ArgumentOutOfRangeException(nameof(responseByteBudget)); _fullRecordCapacity = fullRecordCapacity; _tombstoneCapacity = tombstoneCapacity; + _responseByteBudget = responseByteBudget; _utcNow = utcNow ?? (() => DateTimeOffset.UtcNow); _recordIdFactory = recordIdFactory ?? NewRecordId; } @@ -134,6 +165,15 @@ internal int TombstoneCount get { lock (_records) return _tombstones.Count; } } + /// UTF-16 payload of every retained response — the quantity the byte budget caps. + internal long RetainedResponseBytes + { + get { lock (_records) return _retainedResponseBytes; } + } + + private static long ResponseByteCost(string? serializedResponse) => + serializedResponse is null ? 0L : (long)serializedResponse.Length * sizeof(char); + internal MutationTransactionRecord? GetRecord(string transactionId) { lock (_records) @@ -172,7 +212,8 @@ private static string BuildTransactionIdNonBlankPattern() private static string ScalarHex(int value) => value.ToString("X4", System.Globalization.CultureInfo.InvariantCulture); - /// Reserve a new identity, or resolve it to replay/conflict/expired deterministically. + /// Reserve a new identity, or resolve it to replay/conflict/expired/incomplete + /// deterministically. public MutationTransactionDecision Begin(string transactionId, string requestFingerprint) { var requested = new MutationTransactionIdentity( @@ -193,31 +234,63 @@ public MutationTransactionDecision Begin(string transactionId, string requestFin record.Identity, record.SerializedResponse); - // Per-session dispatch serialization means this cannot occur through Dispatcher; - // retaining a typed conflict makes the component safe if it is called directly. + // The id is bound to this exact request but never recorded a terminal response. + // Per-session dispatch serialization plus Abandon mean this cannot occur through + // Dispatcher; it is reachable when the component is driven directly. Reporting a + // conflict here would be false — the fingerprints match — so it gets its own kind. return new MutationTransactionDecision( - MutationTransactionDecisionKind.Conflict, + MutationTransactionDecisionKind.Incomplete, ExistingIdentity: record.Identity); } if (_tombstones.TryGetValue(transactionId, out var tombstone)) { + if (!string.Equals(tombstone.Identity.RequestFingerprint, requestFingerprint, + StringComparison.Ordinal)) + return new MutationTransactionDecision( + MutationTransactionDecisionKind.Conflict, + ExistingIdentity: tombstone.Identity); return new MutationTransactionDecision( - string.Equals(tombstone.Identity.RequestFingerprint, requestFingerprint, - StringComparison.Ordinal) - ? MutationTransactionDecisionKind.ResultEvicted - : MutationTransactionDecisionKind.Conflict, + tombstone.CompletedAt is null + ? MutationTransactionDecisionKind.Incomplete + : MutationTransactionDecisionKind.ResultEvicted, ExistingIdentity: tombstone.Identity); } var reserved = new MutationTransactionRecord( _recordIdFactory(), requested, _utcNow(), null, null); _records.Add(transactionId, reserved); + _reservationFifo.Enqueue(reserved); + EvictStaleReservations(); return new MutationTransactionDecision( MutationTransactionDecisionKind.Reserved, reserved, requested); } } + /// + /// Release a reservation that will never record a terminal response, keeping the identity + /// bound as an outcome-unknown tombstone rather than stranding it in the live record map. + /// Idempotent, and a no-op once the reservation has completed. + /// + public void Abandon(MutationTransactionRecord reservation) + { + ArgumentNullException.ThrowIfNull(reservation); + lock (_records) + RetireReservation(reservation); + } + + private bool RetireReservation(MutationTransactionRecord reservation) + { + var id = reservation.Identity.TransactionId; + if (!_records.TryGetValue(id, out var current) + || !ReferenceEquals(current, reservation) + || current.SerializedResponse is not null) + return false; + _records.Remove(id); + Entomb(current); + return true; + } + /// Atomically retain the exact response and apply FIFO eviction. public MutationTransactionRecord Complete( MutationTransactionRecord reservation, @@ -242,31 +315,64 @@ public MutationTransactionRecord Complete( }; _records[completed.Identity.TransactionId] = completed; _completedFifo.Enqueue(completed.Identity.TransactionId); + _retainedResponseBytes += ResponseByteCost(serializedResponse); EvictCompletedRecords(); return completed; } } + /// + /// Bound retained responses by BOTH the count cap and the byte budget, oldest first. A single + /// response larger than the whole budget evicts itself rather than raising the ceiling: an + /// identical retry then answers transaction_result_evicted, which is a safe answer, + /// whereas an unbounded retained response is not a bound at all. + /// private void EvictCompletedRecords() { - while (_completedFifo.Count > _fullRecordCapacity) + while (_completedFifo.Count > 0 + && (_completedFifo.Count > _fullRecordCapacity + || _retainedResponseBytes > _responseByteBudget)) { var id = _completedFifo.Dequeue(); - if (!_records.Remove(id, out var evicted) || evicted.CompletedAt is not { } completedAt) - continue; - if (_tombstoneCapacity == 0) continue; - - _tombstones[id] = new MutationTransactionTombstone( - evicted.RecordId, - evicted.Identity, - evicted.StartedAt, - completedAt, - // Eviction must move the identity from a full record to a tombstone as one - // logical operation even when an injected diagnostics clock fails. - UtcNowOr(completedAt)); - _tombstoneFifo.Enqueue(id); + if (!_records.Remove(id, out var evicted)) continue; + // Decrement on every path a retained response leaves _records, or the running total + // drifts upward and the budget silently becomes permanent. + _retainedResponseBytes -= ResponseByteCost(evicted.SerializedResponse); + Entomb(evicted); } + TrimTombstones(); + } + + /// + /// Bound reservations that were never completed or abandoned. Only reachable through direct + /// component use; the Dispatcher always completes or abandons. Identity stays bound as an + /// outcome-unknown tombstone so a stale retry cannot silently become a fresh mutation. + /// + private void EvictStaleReservations() + { + while (_reservationFifo.Count > _fullRecordCapacity) + RetireReservation(_reservationFifo.Dequeue()); + TrimTombstones(); + } + + private void Entomb(MutationTransactionRecord evicted) + { + if (_tombstoneCapacity == 0) return; + var id = evicted.Identity.TransactionId; + _tombstones[id] = new MutationTransactionTombstone( + evicted.RecordId, + evicted.Identity, + evicted.StartedAt, + evicted.CompletedAt, + // Eviction must move the identity from a full record to a tombstone as one + // logical operation even when an injected diagnostics clock fails. + UtcNowOr(evicted.CompletedAt ?? evicted.StartedAt)); + _tombstoneFifo.Enqueue(id); + } + + private void TrimTombstones() + { while (_tombstoneFifo.Count > _tombstoneCapacity) { var id = _tombstoneFifo.Dequeue(); diff --git a/tools/mcp-server/README.md b/tools/mcp-server/README.md index 0885cebf..8b97bb92 100644 --- a/tools/mcp-server/README.md +++ b/tools/mcp-server/README.md @@ -103,11 +103,45 @@ returns the exact original serialized batch result without applying again or rec different request with that id fails with `transaction_conflict`. Results expose `transaction: { schemaVersion: 1, transactionId, requestFingerprint }`. Preview/dry-run batches and -direct or nested step calls reject transaction ids. Retention is bounded per session (128 complete -responses followed by 1,024 response-less tombstones). Save preserves this journal; close clears -it, and reopen starts a new identity namespace. Replay after undo/redo returns the historical -response without changing the document or either history cursor; use ordinary redo to restore an -undone mutation. +direct or nested step calls reject transaction ids — a client that blanket-attaches an idempotency +key to *every* tool call gets a hard error on the other tools, so attach it only to applying +`docxodus_mutations` batches. Save preserves this journal; close clears it, and reopen starts a new +identity namespace. Replay after undo/redo returns the historical response without changing the +document or either history cursor; use ordinary redo to restore an undone mutation. + +### Retention bound and its memory cost + +Retention is bounded per session by **both** a count and a byte budget: at most 128 complete +responses, at most 32 MiB of retained response text, whichever binds first, followed by 1,024 +response-less FIFO tombstones that keep an id bound without holding its payload. A single response +larger than the whole budget evicts itself rather than raising the ceiling. + +The byte budget exists because a count is not a memory bound. A retained entry is the complete +serialized `MutationBatchResult`, which carries every step's `results` **twice** (once under +`steps[].results` and once in the duplicate top-level `results`) plus `patch.markdown` and the +revision/comment/annotation delta sets. Measured: a single-step `insert_paragraph` batch against a +blank document already retains **~3.2 KB**, so 128 of those is ~400 KB — and a large scoped batch +over a real document is orders of magnitude bigger, which is what the 32 MiB cap is there for. The +worst case is therefore **~32 MiB per open session**; the number of open sessions is *not* bounded, +and there is no TTL or idle-session eviction, so a long-lived server that is never sent +`docxodus_close` still grows with the number of sessions. + +### Lifecycle hazards + +- **A validation failure burns the id.** A structured rejection (say `mode: "sideways"`) is itself + a terminal response and is cached. Fixing the typo and resending under the same `transactionId` + gets `transaction_conflict`, not a retry. Always use a **fresh** id after any failure you intend + to correct; reuse an id only for a byte-identical resend of the same request. +- **Tombstone expiry lets a stale retry re-apply.** After roughly 128 further transactions plus + 1,024 more tombstones, an id is genuinely forgotten and a retry that arrives after that becomes a + **fresh mutation that applies again**. Idempotency is a bounded-window guarantee, not a permanent + one; a client holding a request for a long time must not assume the window is still open. +- **`transaction_incomplete`** means the id is bound to this exact request but no terminal response + was ever recorded, so whether the mutation applied is *unknown*. Inspect the document and retry + under a new id. +- **Idempotency is MCP-only.** `execute_batch` through WASM/npm and through the stdio host / + `docx-scalpel` has no transaction identity and no replay: a retry there re-applies. Do not + assume the MCP guarantee from another transport. ## Known gaps diff --git a/tools/mcp-server/SessionStore.cs b/tools/mcp-server/SessionStore.cs index d4a9c7be..1b566c59 100644 --- a/tools/mcp-server/SessionStore.cs +++ b/tools/mcp-server/SessionStore.cs @@ -79,6 +79,10 @@ public DocSession Open(byte[] bytes, string? location, DocxSessionSettings setti // Construct fallible per-session collaborators before allocating a core handle. If the // factory fails, there is nothing in either registry to clean up. var mutationTransactions = _mutationTransactionsFactory(); + // One lifecycle-to-session lock order means the document parse inside OpenSession runs + // under the process-wide lifecycle gate, so opens do not overlap each other. Acceptable + // while both shipped transports are serial anyway; revisit if request handling ever moves + // off the accept loop, since open is the one genuinely slow operation here. lock (_lifecycleGate) { var handle = DocxSessionOps.OpenSession(bytes, settings); diff --git a/tools/mcp-server/ToolCatalog.cs b/tools/mcp-server/ToolCatalog.cs index 714f3d99..3a74107a 100644 --- a/tools/mcp-server/ToolCatalog.cs +++ b/tools/mcp-server/ToolCatalog.cs @@ -509,7 +509,7 @@ internal static class ToolCatalog "type": "object", "properties": { "sessionId": { "type": "string" }, - "transactionId": { "type": "string", "minLength": 1, "maxLength": 256, "pattern": {{JsonSerializer.Serialize(MutationTransactions.TransactionIdNonBlankPattern)}}, "description": {{JsonSerializer.Serialize(MutationTransactions.TransactionIdSchemaDescription)}} }, + "transactionId": { "type": "string", "minLength": 1, "maxLength": {{MutationTransactions.MaxTransactionIdLength}}, "pattern": {{JsonSerializer.Serialize(MutationTransactions.TransactionIdNonBlankPattern)}}, "description": {{JsonSerializer.Serialize(MutationTransactions.TransactionIdSchemaDescription)}} }, "preconditions": { "type": "object", "description": "Optional batch-start guards. Each step args object may also carry its own preconditions." }, "mode": { "type": "string", "enum": ["atomic", "best_effort", "apply", "preview"], "default": "atomic", "description": "atomic (default): all steps commit as one undo/version unit or fully roll back. best_effort: explicitly retain successful steps after failures. apply: deprecated alias for best_effort. preview: isolated dry-run shorthand using atomic policy unless previewPolicy says best_effort." }, "preview": { "type": "boolean", "default": false, "description": "Dry-run mode for mode=atomic or mode=best_effort. The complete package is cloned and the live document, version, caches, configuration, and undo/redo history are never touched." },