Skip to content

Commit bf8fccb

Browse files
HandyS11claude
andcommitted
Fix the cross-cutting clan defects the whole-branch review found
Render an explicit "no pending invites" embed when a clan is stored but has none. An empty payload means "leave the previous message on screen" to both the reconciler and ServerInfoRefresher, so the pinned invites embed listed resolved invitations forever. The two paths that must stay empty — no server id, and no clan stored — are unchanged, since a non-empty payload on a clanless server would trigger a full reconcile on every refresh tick. Isolate each clan state change in its own try/catch inside the consumer loop. The broad catch sat outside the await foreach, so one transient store failure or Discord 5xx ended the loop for the life of the process, after which the clan channels were never created and, worse, never torn down again. Harvest member names from the live team snapshot on the HasClan path, the second of the two name sources the design calls for. Without it a fresh install renders every roster entry, leader, creator and feed line as a bare Steam profile link until that member happens to speak in clan chat. It is best-effort: a disconnected socket or a failure never blocks persistence, the reconcile or the feed. Refuse to build a WorkspaceRegistry whose channel specs name a capability no provider answers for. The reconciler reads "no provider" as "unavailable", and unavailable deletes the channel and its history, so a host composing AddWorkspace() without AddClans() would silently destroy every guild's #clanchat and #claninfo on its first heal. It must fail to start instead. Budget the roster embed's total text. Per-field 1024 truncation left the 6000-char whole-embed cap unguarded, and EmbedBuilder.Build() throws rather than trimming; six role groups at their ceiling breach it. Groups are now added while a running total fits, and a localized notice names how many were omitted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 70374af commit bf8fccb

15 files changed

Lines changed: 506 additions & 31 deletions

File tree

src/RustPlusBot.Features.Clans/Hosting/ClansHostedService.cs

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,25 @@ private async Task ConsumeClanStateAsync(CancellationToken cancellationToken)
5858
await foreach (var evt in eventBus.SubscribeAsync<ClanStateChangedEvent>(cancellationToken)
5959
.ConfigureAwait(false))
6060
{
61-
await stateService.ApplyAsync(evt, cancellationToken).ConfigureAwait(false);
61+
// Per-item isolation: a transient store failure, a Discord 5xx during the transition
62+
// reconcile or an embed validation throw must cost one event, not the loop. Losing
63+
// the loop would also strand the teardown path, leaving the clan channels behind
64+
// for a player who has already left their clan.
65+
try
66+
{
67+
await stateService.ApplyAsync(evt, cancellationToken).ConfigureAwait(false);
68+
}
69+
catch (OperationCanceledException)
70+
{
71+
// Shutting down: let the outer handler end the loop quietly.
72+
throw;
73+
}
74+
#pragma warning disable CA1031 // Broad catch: one faulted clan state change must not end the loop.
75+
catch (Exception ex)
76+
#pragma warning restore CA1031
77+
{
78+
LogStateChangeFailed(logger, evt.GuildId, evt.ServerId, ex);
79+
}
6280
}
6381
}
6482
catch (OperationCanceledException)
@@ -75,4 +93,11 @@ private async Task ConsumeClanStateAsync(CancellationToken cancellationToken)
7593

7694
[LoggerMessage(Level = LogLevel.Error, Message = "Clan state loop faulted.")]
7795
private static partial void LogStateLoopFaulted(ILogger logger, Exception exception);
96+
97+
[LoggerMessage(Level = LogLevel.Error,
98+
Message = "Applying a clan state change for guild {GuildId} server {ServerId} failed.")]
99+
private static partial void LogStateChangeFailed(ILogger logger,
100+
ulong guildId,
101+
Guid serverId,
102+
Exception exception);
78103
}

src/RustPlusBot.Features.Clans/Messages/ClanInvitesMessageRenderer.cs

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,8 @@
1010
namespace RustPlusBot.Features.Clans.Messages;
1111

1212
/// <summary>
13-
/// Renders the anchored #claninfo pending-invites embed. Renders empty when there are no pending
14-
/// invites so the reconciler never posts the message at all.
13+
/// Renders the anchored #claninfo pending-invites embed. Renders empty only where there is no
14+
/// clan to describe, so the reconciler never posts the message at all on a clanless server.
1515
/// </summary>
1616
/// <param name="store">Supplies the stored clan snapshot.</param>
1717
/// <param name="names">Resolves invitee and recruiter Steam ids to display names.</param>
@@ -38,14 +38,20 @@ public async ValueTask<MessagePayload> RenderAsync(MessageRenderContext context,
3838
}
3939

4040
var clan = await store.GetAsync(context.GuildId, serverId, cancellationToken).ConfigureAwait(false);
41-
if (clan is null || clan.Invites.Count == 0)
41+
if (clan is null)
4242
{
43-
// See ClanOverviewMessageRenderer: an empty payload keeps this key inert, both on clanless
44-
// servers and when there is simply nothing pending.
43+
// See ClanOverviewMessageRenderer: an empty payload keeps this key inert on clanless
44+
// servers, where the channel this message would live in does not exist either.
4545
return new MessagePayload(null, null, null);
4646
}
4747

4848
var culture = context.Culture;
49+
if (clan.Invites.Count == 0)
50+
{
51+
// Honest over stale: an empty payload means "leave the previous message on screen", so
52+
// returning one here would keep a since-resolved invite list pinned forever.
53+
return new MessagePayload(null, EmptyState(culture), null);
54+
}
4955

5056
// One batched call covering both sides of every invite.
5157
var ids = new HashSet<ulong>();
@@ -78,6 +84,16 @@ public async ValueTask<MessagePayload> RenderAsync(MessageRenderContext context,
7884
return new MessagePayload(null, embed, null);
7985
}
8086

87+
/// <summary>Builds the "nothing pending" embed shown while a clan is stored but has no invites.</summary>
88+
/// <param name="culture">The guild culture.</param>
89+
/// <returns>The empty-state embed.</returns>
90+
private Embed EmptyState(string culture) =>
91+
new EmbedBuilder()
92+
.WithTitle(localizer.Get("clan.invites.title", culture, "0"))
93+
.WithColor(Color.Gold)
94+
.WithDescription(localizer.Get("clan.invites.none", culture))
95+
.Build();
96+
8197
private static string Name(IReadOnlyDictionary<ulong, string> resolved, ulong steamId) =>
8298
resolved.TryGetValue(steamId, out var name)
8399
? name

src/RustPlusBot.Features.Clans/Messages/ClanRosterMessageRenderer.cs

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,13 @@ public sealed class ClanRosterMessageRenderer(
2727
/// <summary>Discord's hard cap on the length of an embed field value.</summary>
2828
private const int FieldValueLimit = 1024;
2929

30+
/// <summary>
31+
/// Budget for the whole embed's text. Discord's hard cap is 6000 and
32+
/// <see cref="EmbedBuilder.Build" /> throws above it; the margin leaves room for the title and
33+
/// the omission notice, both of which are added after the budget is spent.
34+
/// </summary>
35+
private const int EmbedTextBudget = 5200;
36+
3037
/// <inheritdoc />
3138
public string MessageKey => Key;
3239

@@ -66,6 +73,7 @@ public async ValueTask<MessagePayload> RenderAsync(MessageRenderContext context,
6673
.ConfigureAwait(false);
6774

6875
var roles = clan.Roles.ToDictionary(r => r.RoleId);
76+
var groups = new List<(string Name, string Value)>();
6977
foreach (var role in clan.Roles.OrderBy(r => r.Rank).ThenBy(r => r.RoleId))
7078
{
7179
var members = Ordered(clan.Members.Where(m => m.RoleId == role.RoleId)).ToList();
@@ -74,13 +82,36 @@ public async ValueTask<MessagePayload> RenderAsync(MessageRenderContext context,
7482
continue;
7583
}
7684

77-
embed.AddField(Header(role, culture), Body(members, resolved, culture));
85+
groups.Add((Header(role, culture), Body(members, resolved, culture)));
7886
}
7987

8088
var orphans = Ordered(clan.Members.Where(m => !roles.ContainsKey(m.RoleId))).ToList();
8189
if (orphans.Count > 0)
8290
{
83-
embed.AddField(localizer.Get("clan.roster.unknownrole", culture), Body(orphans, resolved, culture));
91+
groups.Add((localizer.Get("clan.roster.unknownrole", culture),
92+
Body(orphans, resolved, culture)));
93+
}
94+
95+
// Per-field truncation alone is not enough: enough role groups at their 1024-char ceiling
96+
// breach Discord's 6000-char whole-embed cap, and Build() throws rather than trimming.
97+
var spent = 0;
98+
var added = 0;
99+
foreach (var (name, value) in groups)
100+
{
101+
if (spent + name.Length + value.Length > EmbedTextBudget)
102+
{
103+
break;
104+
}
105+
106+
embed.AddField(name, value);
107+
spent += name.Length + value.Length;
108+
added++;
109+
}
110+
111+
if (added < groups.Count)
112+
{
113+
embed.WithDescription(localizer.Get("clan.roster.omitted", culture,
114+
(groups.Count - added).ToString(CultureInfo.InvariantCulture)));
84115
}
85116

86117
return new MessagePayload(null, embed.Build(), null);

src/RustPlusBot.Features.Clans/State/ClanStateService.cs

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,9 @@ private async Task ApplyHasClanAsync(
119119
var previous = await store.GetAsync(evt.GuildId, evt.ServerId, cancellationToken).ConfigureAwait(false);
120120
await store.SaveAsync(evt.GuildId, evt.ServerId, snapshot, cancellationToken).ConfigureAwait(false);
121121

122+
// Harvest before rendering so the feed lines below can already use the learned names.
123+
await RecordTeamNamesAsync(services, store, evt, snapshot, cancellationToken).ConfigureAwait(false);
124+
122125
var changes = ClanSnapshotDiffer.Diff(previous, snapshot);
123126
await PostChangesAsync(services, evt, changes, snapshot, cancellationToken).ConfigureAwait(false);
124127

@@ -131,6 +134,67 @@ private async Task ApplyHasClanAsync(
131134
}
132135
}
133136

137+
/// <summary>
138+
/// Populates the SteamId → Name cache from the live team snapshot, the second of the two name
139+
/// sources (clan chat senders being the first). Without it a fresh install renders every roster
140+
/// entry as a bare profile link until that member happens to speak in clan chat.
141+
/// </summary>
142+
/// <param name="services">The per-call scope's services.</param>
143+
/// <param name="store">The scoped clan store.</param>
144+
/// <param name="evt">The change being applied (identifies the server).</param>
145+
/// <param name="snapshot">The clan snapshot whose roster bounds what is worth recording.</param>
146+
/// <param name="cancellationToken">A cancellation token.</param>
147+
/// <returns>A task that completes once any learned names have been recorded.</returns>
148+
private async Task RecordTeamNamesAsync(
149+
IServiceProvider services,
150+
IClanStore store,
151+
ClanStateChangedEvent evt,
152+
ClanSnapshot snapshot,
153+
CancellationToken cancellationToken)
154+
{
155+
try
156+
{
157+
var query = services.GetRequiredService<IRustServerQuery>();
158+
var team = await query.GetTeamInfoAsync(evt.GuildId, evt.ServerId, cancellationToken)
159+
.ConfigureAwait(false);
160+
if (team is null)
161+
{
162+
// Disconnected: names stay as they are, which is exactly the pre-existing behaviour.
163+
return;
164+
}
165+
166+
var roster = snapshot.Members.Select(m => m.SteamId).ToHashSet();
167+
var candidates = team.Members
168+
.Where(m => roster.Contains(m.SteamId) && !string.IsNullOrWhiteSpace(m.Name))
169+
.ToList();
170+
if (candidates.Count == 0)
171+
{
172+
return;
173+
}
174+
175+
var known = await store
176+
.GetNamesAsync(evt.GuildId, evt.ServerId, candidates.ConvertAll(m => m.SteamId), cancellationToken)
177+
.ConfigureAwait(false);
178+
179+
foreach (var member in candidates.Where(m => !known.ContainsKey(m.SteamId)))
180+
{
181+
await store.RecordNameAsync(evt.GuildId, evt.ServerId, member.SteamId, member.Name,
182+
cancellationToken).ConfigureAwait(false);
183+
}
184+
}
185+
catch (OperationCanceledException)
186+
{
187+
// Shutting down: the caller's own awaits will observe this too.
188+
throw;
189+
}
190+
#pragma warning disable CA1031 // Broad catch: name harvesting is cosmetic and must never block persistence.
191+
catch (Exception ex)
192+
#pragma warning restore CA1031
193+
{
194+
LogNameHarvestFailed(logger, evt.GuildId, evt.ServerId, ex);
195+
}
196+
}
197+
134198
private async Task ReconcileAsync(
135199
IServiceProvider services,
136200
ClanStateChangedEvent evt,
@@ -240,4 +304,11 @@ private List<ClanChange> ApplyScoreThrottle(ClanStateChangedEvent evt, IReadOnly
240304
[LoggerMessage(Level = LogLevel.Warning,
241305
Message = "A HasClan clan state change for guild {GuildId} server {ServerId} carried no snapshot.")]
242306
private static partial void LogMissingSnapshot(ILogger logger, ulong guildId, Guid serverId);
307+
308+
[LoggerMessage(Level = LogLevel.Debug,
309+
Message = "Harvesting clan member names from the team snapshot for guild {GuildId} server {ServerId} failed.")]
310+
private static partial void LogNameHarvestFailed(ILogger logger,
311+
ulong guildId,
312+
Guid serverId,
313+
Exception exception);
243314
}

src/RustPlusBot.Features.Workspace/Registry/WorkspaceRegistry.cs

Lines changed: 42 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,50 @@
11
namespace RustPlusBot.Features.Workspace.Registry;
22

33
/// <summary>Aggregates contributed spec providers into ordered, scope-filtered views.</summary>
4-
/// <param name="channelProviders">All channel spec providers.</param>
5-
/// <param name="messageProviders">All message spec providers.</param>
6-
/// <param name="capabilityProviders">All capability providers, indexed by capability name.</param>
7-
internal sealed class WorkspaceRegistry(
8-
IEnumerable<IChannelSpecProvider> channelProviders,
9-
IEnumerable<IMessageSpecProvider> messageProviders,
10-
IEnumerable<IWorkspaceCapabilityProvider> capabilityProviders) : IWorkspaceRegistry
4+
internal sealed class WorkspaceRegistry : IWorkspaceRegistry
115
{
12-
private readonly Dictionary<string, IWorkspaceCapabilityProvider> _capabilities =
13-
capabilityProviders.ToDictionary(p => p.Capability, StringComparer.Ordinal);
6+
private readonly Dictionary<string, IWorkspaceCapabilityProvider> _capabilities;
7+
private readonly List<ChannelSpec> _channels;
8+
private readonly List<MessageSpec> _messages;
149

15-
private readonly List<ChannelSpec> _channels = [.. channelProviders.SelectMany(p => p.GetChannelSpecs())];
16-
private readonly List<MessageSpec> _messages = [.. messageProviders.SelectMany(p => p.GetMessageSpecs())];
10+
/// <summary>Initializes the registry and validates that every gated channel has a provider.</summary>
11+
/// <param name="channelProviders">All channel spec providers.</param>
12+
/// <param name="messageProviders">All message spec providers.</param>
13+
/// <param name="capabilityProviders">All capability providers, indexed by capability name.</param>
14+
/// <exception cref="InvalidOperationException">
15+
/// A channel spec names a capability that no registered provider answers for.
16+
/// </exception>
17+
public WorkspaceRegistry(
18+
IEnumerable<IChannelSpecProvider> channelProviders,
19+
IEnumerable<IMessageSpecProvider> messageProviders,
20+
IEnumerable<IWorkspaceCapabilityProvider> capabilityProviders)
21+
{
22+
_capabilities = capabilityProviders.ToDictionary(p => p.Capability, StringComparer.Ordinal);
23+
_channels = [.. channelProviders.SelectMany(p => p.GetChannelSpecs())];
24+
_messages = [.. messageProviders.SelectMany(p => p.GetMessageSpecs())];
25+
26+
// Fail the host rather than the guild's data: the reconciler reads "no provider" as "capability
27+
// unavailable", and unavailable means it DELETES the gated channel and everything in it. A host
28+
// that composes AddWorkspace() without the module owning a capability would silently destroy
29+
// those channels on its first heal, so refuse to start instead.
30+
var missing = new SortedSet<string>(StringComparer.Ordinal);
31+
foreach (var spec in _channels)
32+
{
33+
if (spec.Capability is { } capability && !_capabilities.ContainsKey(capability))
34+
{
35+
missing.Add(capability);
36+
}
37+
}
38+
39+
if (missing.Count > 0)
40+
{
41+
throw new InvalidOperationException(
42+
"No IWorkspaceCapabilityProvider is registered for workspace channel capability(ies): " +
43+
string.Join(", ", missing) +
44+
". The host must compose the feature module that provides them; without it the reconciler " +
45+
"would treat the gated channels as unavailable and delete them along with their history.");
46+
}
47+
}
1748

1849
/// <inheritdoc />
1950
public ValueTask<bool> IsCapabilityAvailableAsync(string capability,

src/RustPlusBot.Localization/Strings.fr.resx

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,9 @@
162162
<data name="clan.invites.line" xml:space="preserve">
163163
<value>{0} — invité par {1} · {2}</value>
164164
</data>
165+
<data name="clan.invites.none" xml:space="preserve">
166+
<value>Aucune invitation en attente.</value>
167+
</data>
165168
<data name="clan.invites.title" xml:space="preserve">
166169
<value>📨 Invitations en attente ({0})</value>
167170
</data>
@@ -219,6 +222,9 @@
219222
<data name="clan.roster.notes" xml:space="preserve">
220223
<value>· {0}</value>
221224
</data>
225+
<data name="clan.roster.omitted" xml:space="preserve">
226+
<value>…et {0} groupe(s) de rôles omis pour respecter la limite de taille des embeds Discord</value>
227+
</data>
222228
<data name="clan.roster.perm.accesslogs" xml:space="preserve">
223229
<value>Voir les journaux</value>
224230
</data>

src/RustPlusBot.Localization/Strings.resx

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,9 @@
162162
<data name="clan.invites.line" xml:space="preserve">
163163
<value>{0} — invited by {1} · {2}</value>
164164
</data>
165+
<data name="clan.invites.none" xml:space="preserve">
166+
<value>No pending invites.</value>
167+
</data>
165168
<data name="clan.invites.title" xml:space="preserve">
166169
<value>📨 Pending invites ({0})</value>
167170
</data>
@@ -219,6 +222,9 @@
219222
<data name="clan.roster.notes" xml:space="preserve">
220223
<value>· {0}</value>
221224
</data>
225+
<data name="clan.roster.omitted" xml:space="preserve">
226+
<value>…and {0} more role group(s) omitted to fit Discord's embed size limit</value>
227+
</data>
222228
<data name="clan.roster.perm.accesslogs" xml:space="preserve">
223229
<value>View logs</value>
224230
</data>

tests/RustPlusBot.Features.Clans.Tests/ClansRegistrationTests.cs

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,11 @@
1313
using RustPlusBot.Features.Clans.Posting;
1414
using RustPlusBot.Features.Clans.State;
1515
using RustPlusBot.Features.Clans.Writing;
16+
using RustPlusBot.Features.Workspace;
1617
using RustPlusBot.Features.Workspace.Locating;
1718
using RustPlusBot.Features.Workspace.Reconciler;
1819
using RustPlusBot.Features.Workspace.Registry;
20+
using RustPlusBot.Persistence;
1921
using RustPlusBot.Persistence.Clans;
2022
using RustPlusBot.Persistence.Connections;
2123
using RustPlusBot.Persistence.Workspace;
@@ -81,6 +83,31 @@ public async Task Registers_the_assembly_that_carries_the_clan_interaction_modul
8183
a => a.Assembly == typeof(ClanMotdModule).Assembly);
8284
}
8385

86+
[Fact]
87+
public void The_composed_host_registry_covers_every_gated_channel_capability()
88+
{
89+
// WorkspaceRegistry refuses to build when a ChannelSpec names a capability no provider
90+
// answers for, because the reconciler would otherwise delete the gated channels. The clan
91+
// specs live in Features.Workspace and the provider in Features.Clans, so this pins that the
92+
// real composition of the two satisfies the guard.
93+
var services = new ServiceCollection();
94+
services.AddSingleton(new DiscordSocketClient());
95+
services.AddSingleton<IClock, SystemClock>();
96+
services.AddSingleton<IEventBus, InMemoryEventBus>();
97+
services.AddSingleton(Substitute.For<IRustServerQuery>());
98+
services.AddLogging();
99+
services.AddBotPersistence("DataSource=:memory:");
100+
services.AddWorkspace();
101+
services.AddClans();
102+
103+
using var provider = services.BuildServiceProvider(new ServiceProviderOptions
104+
{
105+
ValidateScopes = true
106+
});
107+
108+
Assert.NotNull(provider.GetRequiredService<IWorkspaceRegistry>());
109+
}
110+
84111
[Fact]
85112
public void Does_not_register_a_second_chat_bridge()
86113
{

0 commit comments

Comments
 (0)