Verlorene Updates erkennen statt warten: optimistische Pruefung beim Ersetzen, Konflikte im Audit und im Lagebild (#720)
FileLedger merkt je Agent die gelesene Fassung einer Datei in gemeinsamen Bereichen. write, copy und Downloads (FTP, RocketChat) ersetzen eine Datei nur, wenn sie seit dem Lesen unveraendert ist; eine fremde Datei nicht ungelesen. append bleibt ungeprueft, macht aeltere Lesestaende aber ungueltig. Abgelehnte Schreibvorgaenge: eigener Audit-Status Conflict, Tabelle FileConflicts, Hotspots (>= 3 in 7 Tagen) in der Teamuebersicht. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Richard
co-authored by
Claude Opus 5.5
Claude Opus 5.5
parent
8a7212c240
commit
79a3a7854f
@@ -162,4 +162,28 @@ public sealed class TeamOverviewTests
|
||||
public void Without_units_there_is_nothing_to_roll_up()
|
||||
=> TeamOverview.RollUp(new OrgChart([], [new AgentConfig { AgentId = "a" }]), [Member("a", null, TeamState.Problem)])
|
||||
.ShouldBeEmpty();
|
||||
|
||||
// ─── #720: conflict hotspots ───
|
||||
|
||||
[Fact]
|
||||
public void A_hotspot_is_shown_relative_to_the_agents_folder()
|
||||
{
|
||||
var agents = Path.Combine(Path.GetTempPath(), "inst", "Agents");
|
||||
var configured = new List<AgentConfig> { new() { AgentId = "a", SharedWorkspacePath = Path.Combine(agents, "SharedWorkspace") } };
|
||||
|
||||
TeamOverview.Readable(Path.Combine(agents, "SharedWorkspaces", "articles", "list.csv"), configured)
|
||||
.ShouldBe("SharedWorkspaces/articles/list.csv");
|
||||
TeamOverview.Readable(Path.Combine(agents, "SharedWorkspace", "plan.md"), configured)
|
||||
.ShouldBe("SharedWorkspace/plan.md");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void A_path_elsewhere_stays_as_it_is()
|
||||
{
|
||||
var configured = new List<AgentConfig> { new() { AgentId = "a", SharedWorkspacePath = Path.Combine(Path.GetTempPath(), "inst", "Agents", "SharedWorkspace") } };
|
||||
var elsewhere = Path.Combine(Path.GetTempPath(), "other", "x.md");
|
||||
|
||||
TeamOverview.Readable(elsewhere, configured).ShouldBe(elsewhere);
|
||||
TeamOverview.Readable(elsewhere, []).ShouldBe(elsewhere);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -0,0 +1,188 @@
|
||||
using ClawdDotNet.Core.Audit;
|
||||
using ClawdDotNet.Core.Files;
|
||||
using ClawdDotNet.Core.Storage;
|
||||
using ClawdDotNet.Core.Tests.Infrastructure;
|
||||
using Shouldly;
|
||||
|
||||
namespace ClawdDotNet.Core.Tests.Files;
|
||||
|
||||
/// <summary>
|
||||
/// #720: The lost update — A reads, B reads, A writes, B writes, and A's change is gone
|
||||
/// without a word. The ledger refuses B's write instead and keeps the conflict for the count.
|
||||
/// </summary>
|
||||
public sealed class FileLedgerTests : IDisposable
|
||||
{
|
||||
private readonly string _dir = Path.Combine(Path.GetTempPath(), "clawd-tests", "ledger-" + Guid.NewGuid().ToString("N")[..8]);
|
||||
private readonly string _file;
|
||||
private readonly FileLedger _ledger;
|
||||
|
||||
public FileLedgerTests()
|
||||
{
|
||||
Directory.CreateDirectory(_dir);
|
||||
_file = Path.Combine(_dir, "plan.md");
|
||||
File.WriteAllText(_file, "v1");
|
||||
_ledger = new FileLedger(new SqliteStorage(Path.Combine(_dir, "state.db")));
|
||||
}
|
||||
|
||||
public void Dispose()
|
||||
{
|
||||
SqliteStorage.ReleasePooledConnections(_dir);
|
||||
try { Directory.Delete(_dir, recursive: true); }
|
||||
catch { /* cleanup is secondary */ }
|
||||
}
|
||||
|
||||
private string? Now => FileLedger.HashOf(_file);
|
||||
|
||||
private void Write(string agent, string content)
|
||||
{
|
||||
File.WriteAllText(_file, content);
|
||||
_ledger.NoteReplace(agent, "run-" + agent, _file, Now);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void The_second_writer_of_the_same_read_is_refused()
|
||||
{
|
||||
_ledger.NoteRead("a", _file, Now);
|
||||
_ledger.NoteRead("b", _file, Now);
|
||||
|
||||
_ledger.CheckReplace("a", "run-a", _file, "shared", Now).ShouldBeNull();
|
||||
Write("a", "v2 by a");
|
||||
|
||||
var conflict = _ledger.CheckReplace("b", "run-b", _file, "shared", Now);
|
||||
conflict.ShouldNotBeNull();
|
||||
conflict.Kind.ShouldBe(FileConflict.ChangedSinceRead);
|
||||
conflict.OtherAgentId.ShouldBe("a");
|
||||
conflict.OtherRunId.ShouldBe("run-a");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void After_reading_again_the_write_goes_through()
|
||||
{
|
||||
_ledger.NoteRead("b", _file, Now);
|
||||
Write("a", "v2 by a");
|
||||
_ledger.CheckReplace("b", null, _file, "shared", Now).ShouldNotBeNull();
|
||||
|
||||
_ledger.NoteRead("b", _file, Now);
|
||||
|
||||
_ledger.CheckReplace("b", null, _file, "shared", Now).ShouldBeNull();
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Writing_twice_in_a_row_is_not_a_conflict_with_oneself()
|
||||
{
|
||||
_ledger.NoteRead("a", _file, Now);
|
||||
Write("a", "v2");
|
||||
|
||||
_ledger.CheckReplace("a", null, _file, "shared", Now).ShouldBeNull();
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Another_agents_file_is_not_overwritten_unread()
|
||||
{
|
||||
Write("a", "a's work");
|
||||
|
||||
var conflict = _ledger.CheckReplace("b", null, _file, "shared", Now);
|
||||
|
||||
conflict.ShouldNotBeNull();
|
||||
conflict.Kind.ShouldBe(FileConflict.NotRead);
|
||||
conflict.Message("shared:/plan.md").ShouldContain("have not read it");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void The_own_file_and_a_file_of_unknown_origin_may_be_overwritten_unread()
|
||||
{
|
||||
Write("a", "a's work");
|
||||
_ledger.CheckReplace("a", null, _file, "shared", Now).ShouldBeNull();
|
||||
|
||||
// A file nobody wrote through the ledger — from before, or by a human.
|
||||
File.WriteAllText(_file, "put there by hand");
|
||||
_ledger.CheckReplace("b", null, _file, "shared", Now).ShouldBeNull();
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void A_new_file_replaces_nothing()
|
||||
=> _ledger.CheckReplace("b", null, Path.Combine(_dir, "new.md"), "shared", null).ShouldBeNull();
|
||||
|
||||
[Fact]
|
||||
public void An_append_by_another_makes_an_older_read_stale()
|
||||
{
|
||||
_ledger.NoteRead("a", _file, Now);
|
||||
|
||||
var before = Now;
|
||||
File.AppendAllText(_file, "\n+ b's line");
|
||||
_ledger.NoteAppend("b", "run-b", _file, before, Now);
|
||||
|
||||
var conflict = _ledger.CheckReplace("a", null, _file, "shared", Now);
|
||||
conflict.ShouldNotBeNull("a's replace would drop b's appended line");
|
||||
conflict.OtherAgentId.ShouldBe("b");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void An_own_append_on_a_known_file_keeps_the_read_valid()
|
||||
{
|
||||
_ledger.NoteRead("a", _file, Now);
|
||||
var before = Now;
|
||||
File.AppendAllText(_file, "\n+ a's line");
|
||||
_ledger.NoteAppend("a", null, _file, before, Now);
|
||||
|
||||
_ledger.CheckReplace("a", null, _file, "shared", Now).ShouldBeNull();
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void A_change_by_hand_after_the_read_is_a_conflict_too()
|
||||
{
|
||||
_ledger.NoteRead("a", _file, Now);
|
||||
File.WriteAllText(_file, "edited by a human");
|
||||
|
||||
var conflict = _ledger.CheckReplace("a", null, _file, "shared", Now);
|
||||
|
||||
conflict.ShouldNotBeNull();
|
||||
conflict.OtherAgentId.ShouldBeNull();
|
||||
conflict.Message("shared:/plan.md").ShouldContain("someone else");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Hotspots_are_files_with_repeated_conflicts()
|
||||
{
|
||||
var other = Path.Combine(_dir, "once.md");
|
||||
for (var i = 0; i < 3; i++)
|
||||
await _ledger.RecordAsync(new FileConflict(_file, "shared", "b", null, "a", null, FileConflict.ChangedSinceRead, DateTime.UtcNow), default);
|
||||
await _ledger.RecordAsync(new FileConflict(other, "shared", "c", null, null, null, FileConflict.NotRead, DateTime.UtcNow), default);
|
||||
await _ledger.RecordAsync(new FileConflict(_file, "shared", "b", null, "a", null, FileConflict.ChangedSinceRead, DateTime.UtcNow.AddDays(-30)), default);
|
||||
|
||||
var hotspots = await _ledger.HotspotsAsync(DateTime.UtcNow.AddDays(-7), minimum: 3, limit: 10, default);
|
||||
|
||||
hotspots.Count.ShouldBe(1);
|
||||
hotspots[0].Path.ShouldBe(_file);
|
||||
hotspots[0].Conflicts.ShouldBe(3, "the one from a month ago is outside the window");
|
||||
hotspots[0].Agents.ShouldBe(["a", "b"]);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task The_engine_audits_a_refused_write_as_a_conflict_not_an_error()
|
||||
{
|
||||
var audit = new InMemoryAuditRepository();
|
||||
var fixture = new EngineFixture(audit).WithTool(FakeTool.Conflicting("Conflict: plan.md was changed"));
|
||||
var agent = fixture.AddAgent("agent-a", "TestTool");
|
||||
fixture.Client.RespondsWithToolCall("TestTool").RespondsWithText("done");
|
||||
|
||||
await fixture.Engine.RunAsync(agent, "go", "test-instance", default);
|
||||
|
||||
audit.Entries.Single(e => e.Status != AuditStatus.Started).Status.ShouldBe(AuditStatus.Conflict);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Tools_find_the_ledger_in_their_context()
|
||||
{
|
||||
Core.Tools.AgentToolContext? seen = null;
|
||||
var fixture = new EngineFixture().WithTool(FakeTool.Observing(c => seen = c));
|
||||
fixture.Engine.Files = _ledger;
|
||||
var agent = fixture.AddAgent("agent-a", "TestTool");
|
||||
fixture.Client.RespondsWithToolCall("TestTool").RespondsWithText("done");
|
||||
|
||||
await fixture.Engine.RunAsync(agent, "go", "test-instance", default);
|
||||
|
||||
seen!.Files.ShouldBeSameAs(_ledger);
|
||||
seen.RunId.ShouldNotBeNullOrWhiteSpace("a conflict names the run");
|
||||
}
|
||||
}
|
||||
@@ -78,6 +78,14 @@ internal sealed class FakeTool : IAgentTool
|
||||
public static FakeTool Failing(string error, string name = "TestTool")
|
||||
=> new(name, (_, _, _) => Task.FromResult(ToolResult.Fail(error)));
|
||||
|
||||
/// <summary>#720: Refuses with a write conflict.</summary>
|
||||
public static FakeTool Conflicting(string error, string name = "TestTool")
|
||||
=> new(name, (_, _, _) => Task.FromResult(ToolResult.Conflict(error)));
|
||||
|
||||
/// <summary>Hands the context it was called with to <paramref name="seen"/>.</summary>
|
||||
public static FakeTool Observing(Action<AgentToolContext> seen, string name = "TestTool")
|
||||
=> new(name, (_, context, _) => { seen(context); return Task.FromResult(ToolResult.Ok("ok")); });
|
||||
|
||||
public static FakeTool Throwing(Exception ex, string name = "TestTool")
|
||||
=> new(name, (_, _, _) => throw ex);
|
||||
|
||||
|
||||
@@ -0,0 +1,141 @@
|
||||
using System.Text.Json;
|
||||
using ClawdDotNet.Core.Files;
|
||||
using ClawdDotNet.Core.Tools;
|
||||
using ClawdDotNet.Tools.FileRW;
|
||||
using Microsoft.Extensions.Logging.Abstractions;
|
||||
using Shouldly;
|
||||
|
||||
namespace ClawdDotNet.Tools.Tests.FileRW;
|
||||
|
||||
/// <summary>
|
||||
/// #720: Two agents on the same file in the shared workspace. Nothing locks across steps;
|
||||
/// the write that would lose the other's change is refused, and the agent reads again.
|
||||
/// </summary>
|
||||
public sealed class LostUpdateTests : IDisposable
|
||||
{
|
||||
private readonly string _agents = Path.Combine(Path.GetTempPath(), "clawd-tests", Guid.NewGuid().ToString("N"), "Agents");
|
||||
private readonly FileRWTool _tool = new();
|
||||
private readonly FileLedger _ledger = new();
|
||||
|
||||
public LostUpdateTests() => Directory.CreateDirectory(Path.Combine(_agents, "SharedWorkspace"));
|
||||
|
||||
public void Dispose()
|
||||
{
|
||||
try { Directory.Delete(Path.GetDirectoryName(_agents)!, recursive: true); }
|
||||
catch { /* cleaning up is secondary */ }
|
||||
}
|
||||
|
||||
private string Shared(string file) => Path.Combine(_agents, "SharedWorkspace", file);
|
||||
|
||||
private Task<ToolResult> Run(string agentId, object input, bool withLedger = true)
|
||||
{
|
||||
var personal = Path.Combine(_agents, agentId, "Workspace");
|
||||
Directory.CreateDirectory(personal);
|
||||
var context = new AgentToolContext(agentId, "inst",
|
||||
new Dictionary<string, object?> { ["sharedAccessLevel"] = "ReadWrite" },
|
||||
new NullStateStore(), NullLogger.Instance, CancellationToken.None,
|
||||
WorkspacePath: personal, SharedWorkspacePath: Path.Combine(_agents, "SharedWorkspace"))
|
||||
{
|
||||
Files = withLedger ? _ledger : null,
|
||||
RunId = "run-" + agentId
|
||||
};
|
||||
return _tool.ExecuteAsync(JsonSerializer.SerializeToElement(input), context, CancellationToken.None);
|
||||
}
|
||||
|
||||
private Task<ToolResult> Read(string agent, string file = "plan.md") => Run(agent, new { action = "read", workspace = "shared", path = file });
|
||||
private Task<ToolResult> Write(string agent, string content, string file = "plan.md") => Run(agent, new { action = "write", workspace = "shared", path = file, content });
|
||||
|
||||
[Fact]
|
||||
public async Task The_second_write_of_the_same_read_is_refused_and_the_first_survives()
|
||||
{
|
||||
await Write("a", "v1");
|
||||
await Read("a");
|
||||
await Read("b");
|
||||
|
||||
(await Write("a", "v2 by a")).Success.ShouldBeTrue();
|
||||
var b = await Write("b", "v2 by b");
|
||||
|
||||
b.Success.ShouldBeFalse();
|
||||
b.IsConflict.ShouldBeTrue();
|
||||
b.ErrorMessage!.ShouldContain("changed by agent 'a' since you read it");
|
||||
File.ReadAllText(Shared("plan.md")).ShouldBe("v2 by a");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Reading_again_resolves_it()
|
||||
{
|
||||
await Write("a", "v1");
|
||||
await Read("b");
|
||||
await Write("a", "v2 by a");
|
||||
(await Write("b", "v2 by b")).IsConflict.ShouldBeTrue();
|
||||
|
||||
(await Read("b")).Content.ShouldContain("v2 by a");
|
||||
(await Write("b", "v3 by b on top of a")).Success.ShouldBeTrue();
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Another_agents_file_is_not_overwritten_blind_but_the_own_is()
|
||||
{
|
||||
await Write("a", "a's report");
|
||||
|
||||
(await Write("a", "a's report, revised")).Success.ShouldBeTrue("the own file may be overwritten without reading");
|
||||
var b = await Write("b", "b's version");
|
||||
|
||||
b.IsConflict.ShouldBeTrue();
|
||||
b.ErrorMessage!.ShouldContain("have not read it");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Appending_is_never_refused_but_makes_older_reads_stale()
|
||||
{
|
||||
await Write("a", "log");
|
||||
await Read("a");
|
||||
|
||||
(await Run("b", new { action = "append", workspace = "shared", path = "plan.md", content = "\nb was here" })).Success.ShouldBeTrue();
|
||||
var a = await Write("a", "log, rewritten");
|
||||
|
||||
a.IsConflict.ShouldBeTrue("the replace would drop b's line");
|
||||
File.ReadAllText(Shared("plan.md")).ShouldContain("b was here");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Copying_over_a_changed_file_is_a_write_like_any_other()
|
||||
{
|
||||
await Write("a", "v1");
|
||||
await Read("b");
|
||||
await Write("a", "v2 by a");
|
||||
await Run("b", new { action = "write", workspace = "shared", path = "draft.md", content = "b's draft" });
|
||||
|
||||
var copy = await Run("b", new { action = "copy", workspace = "shared", path = "draft.md", destinationPath = "plan.md" });
|
||||
|
||||
copy.IsConflict.ShouldBeTrue();
|
||||
File.ReadAllText(Shared("plan.md")).ShouldBe("v2 by a");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task The_personal_workspace_is_not_checked()
|
||||
{
|
||||
await Run("a", new { action = "write", path = "mine.md", content = "v1" });
|
||||
await Run("a", new { action = "read", path = "mine.md" });
|
||||
File.WriteAllText(Path.Combine(_agents, "a", "Workspace", "mine.md"), "changed outside");
|
||||
|
||||
(await Run("a", new { action = "write", path = "mine.md", content = "v2" })).Success.ShouldBeTrue();
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Without_the_ledger_everything_is_as_before()
|
||||
{
|
||||
await Write("a", "v1");
|
||||
(await Run("b", new { action = "write", workspace = "shared", path = "plan.md", content = "b" }, withLedger: false)).Success.ShouldBeTrue();
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Reading_many_counts_as_reading_each()
|
||||
{
|
||||
await Write("a", "one", "x/1.md");
|
||||
await Write("a", "two", "x/2.md");
|
||||
await Run("b", new { action = "read_many", workspace = "shared", path = "x" });
|
||||
|
||||
(await Write("b", "one by b", "x/1.md")).Success.ShouldBeTrue();
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user