fix(privacy): the cleanup could never be applied, and three more from the audit

The cleanup preview marked itself stale before it could be drawn. The
gate bumps a revision on release so a preview cannot survive a wipe; I
then made the preview take the gate, so its own release invalidated it
every single time and the apply button never appeared. The feature has
been shipping non-functional since it was written, with a self-test that
asserted the exact bump that killed it.

Read-only operations no longer move the revision, and preview and
maintenance have their own marks instead of borrowing Cleanup -- which
also stops the five-second metadata refresh from expiring previews, and
stops the UI announcing "another operation is running: cleanup" during a
VACUUM.

The JSON export produced invalid JSON. The chat relation kinds were
interpolated straight into the output, and interpolating an enum writes
its member name, so every message with a recognised relation came out as
"source_kind":LocalPlayer. That is the file a GDPR access request goes
out on. The self-test wrote a JSON file and never parsed it; it does now.

Retention with the limit at zero still deleted. The slider is labelled
"0 = never" and the sweep seeded 31 spec defaults unconditionally before
reading the user's overrides, so zero still lost free company, linkshell
and party history after ninety days -- and the short-circuit written for
exactly this case could never be reached, because the map was never
empty.

A wipe that worked reported that it had failed. VACUUM needs the
database to itself, the refilter walks a lazy reader on the primary
connection outside the lock, and the two collide -- after the DELETE has
committed. The delete paths no longer let that escape: the rows are
gone, an uncompacted file is a housekeeping problem, and telling
somebody their history is still there when it is not is a different kind
of problem.

Also:

- CSV cells starting with =, +, - or @ get a leading apostrophe. The
  content is text other people typed into a chat channel and the file
  exists to be opened in a spreadsheet.
- An export that matched nothing no longer replaces the previous one. It
  used to write its header, move it into place, and then report that
  nothing matched. Dalamud's save dialog offers no overwrite
  confirmation to fall back on, so this is the part that had to move.
- The retention sweep says so when it loses the race for the gate, and
  routes its notifications through the teardown check like everything
  else.
This commit is contained in:
2026-08-19 07:17:33 +02:00
parent 9ea9e96145
commit fdb1a98519
32 changed files with 300 additions and 20 deletions
+28 -2
View File
@@ -92,6 +92,18 @@ internal static class MessageExporter
};
}
// An export that matched nothing does not replace anything. The
// file still has a header and a footer, so moving it would put a
// near-empty file where the user's previous export was -- and then
// report "no message matched the filter", which reads as "nothing
// happened". Dalamud's save dialog has no overwrite confirmation to
// fall back on.
if (written == 0)
{
TryDeleteTemp(temp);
return 0;
}
File.Move(temp, path, overwrite: true);
return written;
}
@@ -225,8 +237,13 @@ internal static class MessageExporter
w.Write($",\"date\":\"{m.Date.ToString("O", CultureInfo.InvariantCulture)}\"");
w.Write($",\"chat_type\":{(int)m.Code.Type}");
w.Write($",\"chat_type_name\":\"{chatType}\"");
w.Write($",\"source_kind\":{m.Code.Source}");
w.Write($",\"target_kind\":{m.Code.Target}");
// Cast, not interpolate. These are XivChatRelationKind, and string
// interpolation of an enum writes the member name -- so every
// message with a recognised relation produced
// "source_kind":LocalPlayer, which no parser accepts. This is the
// file an access request goes out on.
w.Write($",\"source_kind\":{(int)m.Code.Source}");
w.Write($",\"target_kind\":{(int)m.Code.Target}");
w.Write($",\"receiver\":{m.Receiver}");
w.Write($",\"content_id\":{m.ContentId}");
w.Write($",\"sender\":{JsonString(SenderText(m))}");
@@ -313,8 +330,17 @@ internal static class MessageExporter
private static string CsvString(string s)
{
// Leading =, +, - and @ make a spreadsheet treat the cell as a formula.
// Every value here is text somebody else typed into a chat channel, and
// this file exists to be opened in Excel, so a prefixed apostrophe goes
// in front. It is the standard defence and it costs one character that
// spreadsheets hide.
if (s.Length > 0 && s[0] is '=' or '+' or '-' or '@' or '\t' or '\r')
s = "'" + s;
if (s.IndexOfAny(['"', ',', '\n', '\r']) < 0)
return s;
return "\"" + s.Replace("\"", "\"\"") + "\"";
}
}
+31 -4
View File
@@ -493,7 +493,7 @@ internal class MessageStore : IDisposable
{
Connection.Execute("DELETE FROM messages;");
InvalidateFtsIndex();
PerformMaintenance();
TryPerformMaintenance();
}
}
@@ -595,7 +595,7 @@ internal class MessageStore : IDisposable
if (deleted > 0)
{
InvalidateFtsIndex();
PerformMaintenance();
TryPerformMaintenance();
}
return deleted;
@@ -628,7 +628,7 @@ internal class MessageStore : IDisposable
if (deleted > 0)
{
InvalidateFtsIndex();
PerformMaintenance();
TryPerformMaintenance();
}
return deleted;
@@ -663,7 +663,7 @@ internal class MessageStore : IDisposable
if (deleted > 0)
{
InvalidateFtsIndex();
PerformMaintenance();
TryPerformMaintenance();
}
return deleted;
@@ -684,6 +684,33 @@ internal class MessageStore : IDisposable
}
}
// Runs maintenance and swallows a failure, for the delete paths only.
//
// VACUUM needs the database to itself, and a lazily consumed reader on the
// primary connection -- which GetMostRecentMessages hands out and the
// refilter walks outside the lock -- makes it fail immediately with "cannot
// VACUUM - SQL statements in progress". That happens after the DELETE has
// committed, so letting it escape means the caller reports "nothing was
// removed" about a wipe that emptied the database.
//
// The rows are gone either way. An uncompacted file is a housekeeping
// problem; telling somebody their history is still there when it is not is
// a different kind of problem.
private void TryPerformMaintenance()
{
try
{
PerformMaintenance();
}
catch (Exception e)
{
_logger.LogWarning(
e,
"Maintenance after a delete failed; the rows are gone but the file was not compacted."
);
}
}
private string LogPath => DbPath + "-wal";
internal long DatabaseSize() => !File.Exists(DbPath) ? 0 : new FileInfo(DbPath).Length;
+42 -3
View File
@@ -1064,9 +1064,23 @@ public sealed class Plugin : IAsyncDalamudPlugin
return false;
// Snapshot the policy so the user can edit settings while the sweep runs.
//
// Seeded from the spec defaults only when the global limit is not "keep
// forever". The slider is labelled "0 = never", and pre-filling 31
// channels with 365- and 90-day windows made that label a lie: setting
// it to zero still lost free company, linkshell and party history after
// ninety days, and the short-circuit in DeleteByRetentionPolicy could
// never be reached because the map was never empty.
//
// Explicit per-channel overrides still apply. Somebody who typed a
// number for one channel meant that number.
var policy = new Dictionary<int, int>();
foreach (var (type, days) in Privacy.PrivacyDefaults.DefaultRetentionDays)
policy[(int)(ushort)type] = days;
if (Config.RetentionDefaultDays > 0)
{
foreach (var (type, days) in Privacy.PrivacyDefaults.DefaultRetentionDays)
policy[(int)(ushort)type] = days;
}
// This is the enumerator the wizard's Clear() cuts short. Reading under the
// same lock the writers take keeps the policy snapshot whole.
lock (ConfigMapsLock)
@@ -1086,7 +1100,18 @@ public sealed class Plugin : IAsyncDalamudPlugin
try
{
if (!DbOperations.TryBegin(Util.DbOperation.RetentionSweep))
{
// A run the user pressed a button for has to say something.
// The pre-check in StartRetentionSweep only covers a gate
// that was already busy; losing the race here is the same
// outcome and used to be silent.
if (notify)
NotifySweep(
Resources.HellionStrings.Retention_Error,
Dalamud.Interface.ImGuiNotification.NotificationType.Warning
);
return;
}
try
{
@@ -1142,7 +1167,7 @@ public sealed class Plugin : IAsyncDalamudPlugin
{
Log.Error(e, "Retention sweep failed");
if (notify)
Util.WrapperUtil.AddNotification(
NotifySweep(
Resources.HellionStrings.Retention_Error,
Dalamud.Interface.ImGuiNotification.NotificationType.Error
);
@@ -1170,6 +1195,20 @@ public sealed class Plugin : IAsyncDalamudPlugin
}
}
// The sweep is a background thread that can outlive an unload, same as the
// settings-tab workers. A notification filed against a plugin that is gone
// belongs to nobody.
private void NotifySweep(
string message,
Dalamud.Interface.ImGuiNotification.NotificationType type
)
{
if (_isDisposing)
return;
Util.WrapperUtil.AddNotification(message, type);
}
// Read by the settings tab every frame so the manual button can say a run is
// in progress. The gate itself cannot answer that: it goes busy only once
// the worker reaches TryBegin, which is after Start returns.
+2
View File
@@ -277,6 +277,8 @@ internal class HellionStrings
internal static string Settings_Database_Op_Export => Get(nameof(Settings_Database_Op_Export));
internal static string Settings_Database_Op_Cleanup => Get(nameof(Settings_Database_Op_Cleanup));
internal static string Settings_Database_Op_Clear => Get(nameof(Settings_Database_Op_Clear));
internal static string Settings_Database_Op_Preview => Get(nameof(Settings_Database_Op_Preview));
internal static string Settings_Database_Op_Maintenance => Get(nameof(Settings_Database_Op_Maintenance));
// Hellion Chat — Default tab presets (channel-themed)
internal static string Tabs_Presets_System => Get(nameof(Tabs_Presets_System));
@@ -1328,4 +1328,10 @@
<data name="Settings_Tabs_Duplicate" xml:space="preserve">
<value>Duplica</value>
</data>
<data name="Settings_Database_Op_Preview" xml:space="preserve">
<value>vista prèvia</value>
</data>
<data name="Settings_Database_Op_Maintenance" xml:space="preserve">
<value>manteniment</value>
</data>
</root>
@@ -1327,4 +1327,10 @@
<data name="Settings_Tabs_Duplicate" xml:space="preserve">
<value>Duplikovat</value>
</data>
<data name="Settings_Database_Op_Preview" xml:space="preserve">
<value>náhled</value>
</data>
<data name="Settings_Database_Op_Maintenance" xml:space="preserve">
<value>údržba</value>
</data>
</root>
@@ -1327,4 +1327,10 @@
<data name="Settings_Tabs_Duplicate" xml:space="preserve">
<value>Duplikér</value>
</data>
<data name="Settings_Database_Op_Preview" xml:space="preserve">
<value>forhåndsvisning</value>
</data>
<data name="Settings_Database_Op_Maintenance" xml:space="preserve">
<value>vedligeholdelse</value>
</data>
</root>
@@ -1322,4 +1322,10 @@
<data name="Settings_Tabs_Duplicate" xml:space="preserve">
<value>Duplizieren</value>
</data>
<data name="Settings_Database_Op_Preview" xml:space="preserve">
<value>Vorschau</value>
</data>
<data name="Settings_Database_Op_Maintenance" xml:space="preserve">
<value>Wartung</value>
</data>
</root>
@@ -1327,4 +1327,10 @@
<data name="Settings_Tabs_Duplicate" xml:space="preserve">
<value>Αντιγραφή</value>
</data>
<data name="Settings_Database_Op_Preview" xml:space="preserve">
<value>προεπισκόπηση</value>
</data>
<data name="Settings_Database_Op_Maintenance" xml:space="preserve">
<value>συντήρηση</value>
</data>
</root>
@@ -1328,4 +1328,10 @@
<data name="Settings_Tabs_Duplicate" xml:space="preserve">
<value>Duplicar</value>
</data>
<data name="Settings_Database_Op_Preview" xml:space="preserve">
<value>vista previa</value>
</data>
<data name="Settings_Database_Op_Maintenance" xml:space="preserve">
<value>mantenimiento</value>
</data>
</root>
@@ -1327,4 +1327,10 @@
<data name="Settings_Tabs_Duplicate" xml:space="preserve">
<value>Kahdenna</value>
</data>
<data name="Settings_Database_Op_Preview" xml:space="preserve">
<value>esikatselu</value>
</data>
<data name="Settings_Database_Op_Maintenance" xml:space="preserve">
<value>ylläpito</value>
</data>
</root>
@@ -1328,4 +1328,10 @@
<data name="Settings_Tabs_Duplicate" xml:space="preserve">
<value>Dupliquer</value>
</data>
<data name="Settings_Database_Op_Preview" xml:space="preserve">
<value>aperçu</value>
</data>
<data name="Settings_Database_Op_Maintenance" xml:space="preserve">
<value>maintenance</value>
</data>
</root>
@@ -1327,4 +1327,10 @@
<data name="Settings_Tabs_Duplicate" xml:space="preserve">
<value>Másolat</value>
</data>
<data name="Settings_Database_Op_Preview" xml:space="preserve">
<value>előnézet</value>
</data>
<data name="Settings_Database_Op_Maintenance" xml:space="preserve">
<value>karbantartás</value>
</data>
</root>
@@ -1328,4 +1328,10 @@
<data name="Settings_Tabs_Duplicate" xml:space="preserve">
<value>Duplica</value>
</data>
<data name="Settings_Database_Op_Preview" xml:space="preserve">
<value>anteprima</value>
</data>
<data name="Settings_Database_Op_Maintenance" xml:space="preserve">
<value>manutenzione</value>
</data>
</root>
@@ -1328,4 +1328,10 @@
<data name="Settings_Tabs_Duplicate" xml:space="preserve">
<value>複製</value>
</data>
<data name="Settings_Database_Op_Preview" xml:space="preserve">
<value>プレビュー</value>
</data>
<data name="Settings_Database_Op_Maintenance" xml:space="preserve">
<value>メンテナンス</value>
</data>
</root>
@@ -1328,4 +1328,10 @@
<data name="Settings_Tabs_Duplicate" xml:space="preserve">
<value>복제</value>
</data>
<data name="Settings_Database_Op_Preview" xml:space="preserve">
<value>미리보기</value>
</data>
<data name="Settings_Database_Op_Maintenance" xml:space="preserve">
<value>유지 관리</value>
</data>
</root>
@@ -1327,4 +1327,10 @@
<data name="Settings_Tabs_Duplicate" xml:space="preserve">
<value>Dupliser</value>
</data>
<data name="Settings_Database_Op_Preview" xml:space="preserve">
<value>forhåndsvisning</value>
</data>
<data name="Settings_Database_Op_Maintenance" xml:space="preserve">
<value>vedlikehold</value>
</data>
</root>
@@ -1328,4 +1328,10 @@
<data name="Settings_Tabs_Duplicate" xml:space="preserve">
<value>Dupliceren</value>
</data>
<data name="Settings_Database_Op_Preview" xml:space="preserve">
<value>voorbeeld</value>
</data>
<data name="Settings_Database_Op_Maintenance" xml:space="preserve">
<value>onderhoud</value>
</data>
</root>
@@ -1327,4 +1327,10 @@
<data name="Settings_Tabs_Duplicate" xml:space="preserve">
<value>Duplikuj</value>
</data>
<data name="Settings_Database_Op_Preview" xml:space="preserve">
<value>podgląd</value>
</data>
<data name="Settings_Database_Op_Maintenance" xml:space="preserve">
<value>konserwacja</value>
</data>
</root>
@@ -1328,4 +1328,10 @@
<data name="Settings_Tabs_Duplicate" xml:space="preserve">
<value>Duplicar</value>
</data>
<data name="Settings_Database_Op_Preview" xml:space="preserve">
<value>pré-visualização</value>
</data>
<data name="Settings_Database_Op_Maintenance" xml:space="preserve">
<value>manutenção</value>
</data>
</root>
@@ -1327,4 +1327,10 @@
<data name="Settings_Tabs_Duplicate" xml:space="preserve">
<value>Duplicar</value>
</data>
<data name="Settings_Database_Op_Preview" xml:space="preserve">
<value>pré-visualização</value>
</data>
<data name="Settings_Database_Op_Maintenance" xml:space="preserve">
<value>manutenção</value>
</data>
</root>
@@ -1339,4 +1339,10 @@
<data name="Settings_Tabs_Duplicate" xml:space="preserve">
<value>Duplicate</value>
</data>
<data name="Settings_Database_Op_Preview" xml:space="preserve">
<value>preview</value>
</data>
<data name="Settings_Database_Op_Maintenance" xml:space="preserve">
<value>maintenance</value>
</data>
</root>
@@ -1328,4 +1328,10 @@
<data name="Settings_Tabs_Duplicate" xml:space="preserve">
<value>Duplică</value>
</data>
<data name="Settings_Database_Op_Preview" xml:space="preserve">
<value>previzualizare</value>
</data>
<data name="Settings_Database_Op_Maintenance" xml:space="preserve">
<value>întreținere</value>
</data>
</root>
@@ -1328,4 +1328,10 @@
<data name="Settings_Tabs_Duplicate" xml:space="preserve">
<value>Дублировать</value>
</data>
<data name="Settings_Database_Op_Preview" xml:space="preserve">
<value>предпросмотр</value>
</data>
<data name="Settings_Database_Op_Maintenance" xml:space="preserve">
<value>обслуживание</value>
</data>
</root>
@@ -1328,4 +1328,10 @@
<data name="Settings_Tabs_Duplicate" xml:space="preserve">
<value>Duplicera</value>
</data>
<data name="Settings_Database_Op_Preview" xml:space="preserve">
<value>förhandsgranskning</value>
</data>
<data name="Settings_Database_Op_Maintenance" xml:space="preserve">
<value>underhåll</value>
</data>
</root>
@@ -1327,4 +1327,10 @@
<data name="Settings_Tabs_Duplicate" xml:space="preserve">
<value>Çoğalt</value>
</data>
<data name="Settings_Database_Op_Preview" xml:space="preserve">
<value>önizleme</value>
</data>
<data name="Settings_Database_Op_Maintenance" xml:space="preserve">
<value>bakım</value>
</data>
</root>
@@ -1327,4 +1327,10 @@
<data name="Settings_Tabs_Duplicate" xml:space="preserve">
<value>Дублювати</value>
</data>
<data name="Settings_Database_Op_Preview" xml:space="preserve">
<value>попередній перегляд</value>
</data>
<data name="Settings_Database_Op_Maintenance" xml:space="preserve">
<value>обслуговування</value>
</data>
</root>
@@ -1328,4 +1328,10 @@
<data name="Settings_Tabs_Duplicate" xml:space="preserve">
<value>复制</value>
</data>
<data name="Settings_Database_Op_Preview" xml:space="preserve">
<value>预览</value>
</data>
<data name="Settings_Database_Op_Maintenance" xml:space="preserve">
<value>维护</value>
</data>
</root>
@@ -1328,4 +1328,10 @@
<data name="Settings_Tabs_Duplicate" xml:space="preserve">
<value>複製</value>
</data>
<data name="Settings_Database_Op_Preview" xml:space="preserve">
<value>預覽</value>
</data>
<data name="Settings_Database_Op_Maintenance" xml:space="preserve">
<value>維護</value>
</data>
</root>
@@ -125,6 +125,23 @@ internal sealed class ExportRoundTripStep : ISelfTestStep
failures.Add("export produced no file");
if (File.Exists(path + ".part"))
failures.Add("temporary file left behind after a successful export");
// Parsed, not merely counted. The writer builds JSON by hand, and it
// shipped a build where the chat relation kinds were interpolated as
// enum names -- "source_kind":LocalPlayer -- which every parser
// rejects. A test that only checks the file exists would have passed.
try
{
using var doc = System.Text.Json.JsonDocument.Parse(File.ReadAllText(path));
if (!doc.RootElement.TryGetProperty("messages", out var messages))
failures.Add("export JSON has no messages array");
else if (messages.GetArrayLength() != 1)
failures.Add($"export JSON holds {messages.GetArrayLength()} messages, expected 1");
}
catch (System.Text.Json.JsonException e)
{
failures.Add($"export JSON does not parse: {e.Message}");
}
}
// Built with empty SeStrings on purpose: the whole point is that the text
@@ -140,10 +140,11 @@ internal sealed class DataPrivacyTab
v => Plugin.Config.RetentionEnabled = v
);
// Down to 0, which DeleteByRetentionPolicy reads as "keep forever"
// for channels without an override. The slider started at 1, so the
// one value that means "never delete anything" was unreachable
// through the UI while the label promised it.
// Down to 0, which the sweep reads as "keep forever" for every
// channel without an explicit override. That only became true in
// v1.12.0: the sweep used to seed the spec defaults unconditionally,
// so zero still deleted free company and linkshell history after
// ninety days while this label said otherwise.
_w.SliderIntRow(
ImGui.GetID("privacy.logging.default"u8),
HellionStrings.Retention_Default_Label,
@@ -474,7 +475,7 @@ internal sealed class DataPrivacyTab
// MessageCount holds _readLock, and a VACUUM starting under it
// is exactly the collision this gate was written for -- being a
// read rather than a write does not exempt it.
if (!_plugin.DbOperations.TryBegin(DbOperation.Cleanup))
if (!_plugin.DbOperations.TryBegin(DbOperation.Preview))
return;
try
@@ -487,7 +488,7 @@ internal sealed class DataPrivacyTab
}
finally
{
_plugin.DbOperations.End(DbOperation.Cleanup);
_plugin.DbOperations.End(DbOperation.Preview);
}
}
catch (Exception e)
@@ -692,7 +693,7 @@ internal sealed class DataPrivacyTab
{
try
{
if (!_plugin.DbOperations.TryBegin(DbOperation.Cleanup))
if (!_plugin.DbOperations.TryBegin(DbOperation.Maintenance))
{
// Said out loud, like every other refusal. A developer tool
// that silently does nothing is how you end up debugging the
@@ -707,7 +708,7 @@ internal sealed class DataPrivacyTab
}
finally
{
_plugin.DbOperations.End(DbOperation.Cleanup);
_plugin.DbOperations.End(DbOperation.Maintenance);
}
}
catch (Exception e)
@@ -880,7 +881,7 @@ internal sealed class DataPrivacyTab
{
try
{
if (!_plugin.DbOperations.TryBegin(DbOperation.Cleanup))
if (!_plugin.DbOperations.TryBegin(DbOperation.Preview))
{
NotifyBusy();
return;
@@ -892,7 +893,7 @@ internal sealed class DataPrivacyTab
}
finally
{
_plugin.DbOperations.End(DbOperation.Cleanup);
_plugin.DbOperations.End(DbOperation.Preview);
}
}
catch (Exception e)
@@ -1235,6 +1236,8 @@ internal sealed class DataPrivacyTab
DbOperation.Export => HellionStrings.Settings_Database_Op_Export,
DbOperation.Cleanup => HellionStrings.Settings_Database_Op_Cleanup,
DbOperation.Clear => HellionStrings.Settings_Database_Op_Clear,
DbOperation.Preview => HellionStrings.Settings_Database_Op_Preview,
DbOperation.Maintenance => HellionStrings.Settings_Database_Op_Maintenance,
_ => string.Empty,
};
+17 -1
View File
@@ -8,6 +8,12 @@ internal enum DbOperation
Export,
Cleanup,
Clear,
// Read-only, but they hold the store long enough to matter: the preview
// scans every row, the metadata read takes the read lock, and maintenance
// rewrites the file without touching a single row.
Preview,
Maintenance,
}
// One gate for every operation that holds the message store for longer than a
@@ -47,6 +53,16 @@ internal sealed class DbOperationGate
internal long Revision => Interlocked.Read(ref _revision);
// Which operations can change what a preview counted. Getting this wrong in
// the permissive direction only costs a needless recount; getting it wrong
// the other way lets somebody confirm a number that is no longer true.
//
// The preview itself must not be in here, and that is not a detail: it takes
// the gate, so counting its own release would mark every preview stale the
// instant it finished and the apply button would never appear.
private static bool Mutates(DbOperation operation) =>
operation is DbOperation.RetentionSweep or DbOperation.Cleanup or DbOperation.Clear;
internal bool IsBusy => _current != DbOperation.None;
// False when another operation already owns the store. Callers must not
@@ -84,7 +100,7 @@ internal sealed class DbOperationGate
return;
_current = DbOperation.None;
if (operation != DbOperation.Export)
if (Mutates(operation))
Interlocked.Increment(ref _revision);
}
}