Skip to content

Commit ffc1923

Browse files
committed
Fall through when the tray has gone
PendingFiles routes on DiffEngineTray.IsRunning, which a type initialiser reads once. CLAUDE.md documents the late-starting tray - a tray that appears after the test host, which the queue owner branch already handles. This is the mirror case, and nothing handled it. A tray that exits while a long lived host keeps running leaves that cached answer saying a tray is there. The piper send then went to a port nobody was listening on, and returned void: the refusal was swallowed into a trace line and the caller carried on believing it had handed the file over. Every later move and delete for the life of that process was pending in nothing at all - no fallback to the queue owner, and no LaunchDelete either. Have the piper send report whether it connected, and treat a refusal the same as no tray. That is the branch that already exists and already knows what to do.
1 parent 4fec40a commit ffc1923

3 files changed

Lines changed: 113 additions & 14 deletions

File tree

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
// DiffEngineTray is the obsolete public shim, but its IsRunning is still where the tray check
2+
// lives, and this test has to move it.
3+
#pragma warning disable CS0618
4+
5+
/// <summary>
6+
/// Where a pending file goes when the tray check is stale.
7+
/// <para>
8+
/// DiffEngineTray.IsRunning is read once, when the type initialises. A tray that exits while a
9+
/// long lived host keeps running leaves that answer saying a tray is there, so the piper send went
10+
/// to a port nobody was listening on - and, returning nothing, was swallowed into a trace line.
11+
/// The move or delete was then pending in nothing at all: no fallback to the queue owner, no
12+
/// LaunchDelete.
13+
/// </para>
14+
/// </summary>
15+
[NotInParallel]
16+
public class PendingFilesFallbackTests
17+
{
18+
[Test]
19+
public async Task ADeadPiperFallsThroughToTheQueueOwner()
20+
{
21+
await Assert.That(ViewerServer.TryBind(0, out var bound)).IsTrue();
22+
using var server = bound!;
23+
using var cancel = new CancelSource();
24+
25+
var heard = new ConcurrentBag<string>();
26+
var listening = server.Listen(
27+
_ =>
28+
{
29+
heard.Add($"{_.Verb}:{_.Key}");
30+
return ViewerResponse.Success();
31+
},
32+
cancel.Token);
33+
34+
var previousPort = PiperClient.Port;
35+
var previousViewerPort = Environment.GetEnvironmentVariable(ViewerClient.PortVariable);
36+
var previousRunning = DiffEngineTray.IsRunning;
37+
try
38+
{
39+
// A tray that says it is running, on a port nothing is listening on
40+
PiperClient.Port = DeadPort();
41+
DiffEngineTray.IsRunning = true;
42+
Environment.SetEnvironmentVariable(ViewerClient.PortVariable, server.Port.ToString());
43+
44+
PendingFiles.AddMove("temp.txt", "target.txt", null, null, false, null);
45+
await PendingFilesAddDelete("gone.txt");
46+
47+
await Assert.That(heard).Contains(_ => _.StartsWith("Move:", StringComparison.Ordinal));
48+
await Assert.That(heard).Contains(_ => _.StartsWith("Delete:", StringComparison.Ordinal));
49+
}
50+
finally
51+
{
52+
PiperClient.Port = previousPort;
53+
DiffEngineTray.IsRunning = previousRunning;
54+
Environment.SetEnvironmentVariable(ViewerClient.PortVariable, previousViewerPort);
55+
await cancel.CancelAsync();
56+
try
57+
{
58+
await listening.WaitAsync(TimeSpan.FromSeconds(5));
59+
}
60+
catch (Exception exception)
61+
when (exception is OperationCanceledException or TimeoutException)
62+
{
63+
}
64+
}
65+
}
66+
67+
// The delete path launches a viewer when nothing answers, so it is only safe to exercise with
68+
// an owner bound - which is the point of the test.
69+
static Task PendingFilesAddDelete(string file)
70+
{
71+
PendingFiles.AddDelete(file);
72+
return Task.CompletedTask;
73+
}
74+
75+
static int DeadPort()
76+
{
77+
var listener = new TcpListener(IPAddress.Loopback, 0);
78+
listener.Start();
79+
var port = ((IPEndPoint) listener.LocalEndpoint).Port;
80+
listener.Stop();
81+
return port;
82+
}
83+
}

src/DiffEngine/Tray/PendingFiles.cs

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -24,14 +24,21 @@ namespace DiffEngine;
2424
/// process's life, and its moves and deletes arrive here instead — which a tray that owns the
2525
/// queue answers, so they end up tracked either way.
2626
/// </para>
27+
/// <para>
28+
/// The mirror of that case is a tray that exits while a long lived host keeps running, and it is
29+
/// why the piper send is asked whether it connected rather than told to get on with it. The cached
30+
/// answer still says a tray is there, so every later move and delete went to a port nobody was
31+
/// listening on and was swallowed into a trace line: pending in nothing, with no fallback and no
32+
/// LaunchDelete. A refused piper send now falls through to the same branch as no tray at all.
33+
/// </para>
2734
/// </summary>
2835
static class PendingFiles
2936
{
3037
public static void AddDelete(string file)
3138
{
32-
if (DiffEngineTray.IsRunning)
39+
if (DiffEngineTray.IsRunning &&
40+
PiperClient.SendDelete(file))
3341
{
34-
PiperClient.SendDelete(file);
3542
return;
3643
}
3744

@@ -45,9 +52,9 @@ public static void AddDelete(string file)
4552

4653
public static async Task AddDeleteAsync(string file, Cancel cancel)
4754
{
48-
if (DiffEngineTray.IsRunning)
55+
if (DiffEngineTray.IsRunning &&
56+
await PiperClient.SendDeleteAsync(file, cancel))
4957
{
50-
await PiperClient.SendDeleteAsync(file, cancel);
5158
return;
5259
}
5360

@@ -67,9 +74,9 @@ public static void AddMove(
6774
bool canKill,
6875
int? processId)
6976
{
70-
if (DiffEngineTray.IsRunning)
77+
if (DiffEngineTray.IsRunning &&
78+
PiperClient.SendMove(tempFile, targetFile, exe, arguments, canKill, processId))
7179
{
72-
PiperClient.SendMove(tempFile, targetFile, exe, arguments, canKill, processId);
7380
return;
7481
}
7582

@@ -85,9 +92,9 @@ public static async Task AddMoveAsync(
8592
int? processId,
8693
Cancel cancel)
8794
{
88-
if (DiffEngineTray.IsRunning)
95+
if (DiffEngineTray.IsRunning &&
96+
await PiperClient.SendMoveAsync(tempFile, targetFile, exe, arguments, canKill, processId, cancel))
8997
{
90-
await PiperClient.SendMoveAsync(tempFile, targetFile, exe, arguments, canKill, processId, cancel);
9198
return;
9299
}
93100

src/DiffEngine/Tray/PiperClient.cs

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,10 @@ static class PiperClient
22
{
33
public static int Port = 3492;
44

5-
public static void SendDelete(string file) =>
5+
public static bool SendDelete(string file) =>
66
Send(BuildDeletePayload(file));
77

8-
public static Task SendDeleteAsync(
8+
public static Task<bool> SendDeleteAsync(
99
string file,
1010
Cancel cancel = default)
1111
{
@@ -22,7 +22,7 @@ static string BuildDeletePayload(string file) =>
2222
2323
""";
2424

25-
public static void SendMove(
25+
public static bool SendMove(
2626
string tempFile,
2727
string targetFile,
2828
string? exe,
@@ -31,7 +31,7 @@ public static void SendMove(
3131
int? processId) =>
3232
Send(BuildMovePayload(tempFile, targetFile, exe, arguments, canKill, processId));
3333

34-
public static Task SendMoveAsync(
34+
public static Task<bool> SendMoveAsync(
3535
string tempFile,
3636
string targetFile,
3737
string? exe,
@@ -79,28 +79,37 @@ public static string BuildMovePayload(string tempFile, string targetFile, string
7979
return builder.ToString();
8080
}
8181

82-
static void Send(string payload)
82+
/// <summary>
83+
/// True when the tray took it. False is not fatal on its own - the payload is traced either
84+
/// way - but it is what lets the caller send the pending file somewhere else instead of
85+
/// dropping it, which is what happened when this returned nothing.
86+
/// </summary>
87+
static bool Send(string payload)
8388
{
8489
try
8590
{
8691
InnerSend(payload);
92+
return true;
8793
}
8894
catch (Exception exception)
8995
{
9096
HandleSendException(payload, exception);
97+
return false;
9198
}
9299
}
93100

94-
static async Task SendAsync(string payload, Cancel cancel)
101+
static async Task<bool> SendAsync(string payload, Cancel cancel)
95102
{
96103
try
97104
{
98105
await InnerSendAsync(payload, cancel);
106+
return true;
99107
}
100108
// Let cancellation surface to the caller; only genuine send failures are swallowed.
101109
catch (Exception exception) when (exception is not OperationCanceledException)
102110
{
103111
HandleSendException(payload, exception);
112+
return false;
104113
}
105114
}
106115

0 commit comments

Comments
 (0)