Commit graph

2 commits

Author SHA1 Message Date
BruebachL
00151380e3
[CardDB] Break the CardInfo <-> CardSet cycle on reload (#7645)
Some checks are pending
CodeQL / Analyze (cpp) (push) Waiting to run
CodeQL / Analyze (actions) (push) Waiting to run
Build Desktop / Configure (push) Waiting to run
Build Desktop / Debian 13 (push) Blocked by required conditions
Build Desktop / Debian 12 (push) Blocked by required conditions
Build Desktop / Fedora 44 (push) Blocked by required conditions
Build Desktop / Fedora 43 (push) Blocked by required conditions
Build Desktop / Servatrice_Debian 12 (push) Blocked by required conditions
Build Desktop / Ubuntu 24.04 (push) Blocked by required conditions
Build Desktop / Ubuntu 26.04 (push) Blocked by required conditions
Build Desktop / Arch (push) Blocked by required conditions
Build Desktop / Ubuntu 26.04 cover (push) Blocked by required conditions
Build Desktop / macOS 13 Intel (push) Blocked by required conditions
Build Desktop / macOS 14 (push) Blocked by required conditions
Build Desktop / macOS 15 (push) Blocked by required conditions
Build Desktop / macOS 26 Debug (push) Blocked by required conditions
Build Desktop / Windows 10 (push) Blocked by required conditions
Build Desktop / Prune caches (push) Waiting to run
Build Docker / Servatrice (arm) (push) Waiting to run
Build Docker / Servatrice (x86) (push) Waiting to run
Build Docker / Publish multi-platform Servatrice image (push) Blocked by required conditions
* [CardDB] Drop the CardInfo self pointer

The weak self pointer had to be assigned right after every construction,
and a missed assignment silently stopped change notifications. CardInfo
is always owned by a QSharedPointer, so it now derives from
QEnableSharedFromThis<CardInfo> and asks for sharedFromThis() instead:
nothing to initialize, and the CardSet cycle stays broken.

CardSet::newInstance() also loses the commented-out self-pointer
assignment left over from the same design.

* [CardDB] Pin the cardInfoChanged contract without the self pointer

Dropping CardInfo's self pointer moved change notifications onto
QEnableSharedFromThis, so the signal now rests on the card being owned by
a QSharedPointer. Two tests pin that down:

- A property change on a card owned by a CardDatabase still emits
  cardInfoChanged() carrying that card, which is what the card database
  views and models listen to.
- clone() starts out unowned, so a change to the clone is reported as the
  clone's and must not reach the original's observers.

* [CardDB] Treat an unowned CardInfo as a bug, not a no-op

Every CardInfo is built inside a QSharedPointer (both factory overloads
and clone()), and no constructor notifies anyone, so sharedFromThis() is
always available by the time addToSet() or a setter runs. Assert that and
warn in a released build, instead of silently skipping the notification
or the set append.

The copy constructor keeps its explicit QEnableSharedFromThis
initializer: GCC's -Wextra warns about the uninitialized base in every
translation unit that copies a CardInfo otherwise. A comment there
records that the clone deliberately starts unowned.

* [CardDB] Break the CardInfo <-> CardSet cycle on reload

CardSet is a QList<CardInfoPtr> and CardInfo::addToSet() appends the card
to its sets, while a card keeps a CardSetPtr per printing in
setsToPrintings. The sets therefore hold strong references back to the
cards, and the previous database snapshot stayed alive after a reload, so
RSS grew by roughly a second database on every reload.

Empty the sets before the cards are dropped: CardDatabase::clear() and
swapInDatabaseData() clear the sets before replacing the containers, and
removeCard() detaches the card from its sets, so nothing keeps a card
alive on behalf of a set once the database lets go of it.

Two tests pin the behaviour: a reload and a clear() must drop the last
strong reference to a card from the previous snapshot.

* [CardDB] Keep the set detach in one place

clear() and swapInDatabaseData() both emptied every set before dropping the
cards, so share that as detachSetMembership() instead of two copies that have
to stay in sync.

removeCard() now detaches through the first printing of each set directly, the
same way CardInfo::newInstance() appends, instead of an inner loop that always
breaks after the first item.

Because clear() empties the sets up front, its per-card detach was walking
every card's printings to remove it from sets that were already empty. Split
removeCard() into the per-card detach and unindexCard(), and let clear() call
only the latter.

* [CardDB] Test that removeCard detaches the card from its set

removeCard() is the one teardown path a caller can trigger for a single card
(the token editor uses it), and it now detaches the card from its sets as
well. Without that detach the set's strong CardInfoPtr keeps the removed card
alive forever, which is exactly what the reload and clear tests pin for the
whole-snapshot paths.

The test loads the fixture, removes Cat, and requires the card to be gone from
the CAT set and to be destroyed once the test drops its own reference.

* [Servatrice] Drop the settings cache from the logger

#7602 removed #include "settingscache.h" from server_logger.cpp but left
isLoggingEnabled() reading settingsCache, so master no longer compiles: the
servatrice target fails on the undeclared global and server_logger_test, which
compiles the same file without main.cpp, fails to link it.

The same commit made reloadConfig() the single place that stores writelog and
the log filters, precisely so logging threads stop reading the global QSettings.
isLoggingEnabled() now mirrors logMessage() and reads those members instead: the
console is a sink of its own, and the logfile needs an open file plus writelog.
The mutex is mutable so the const query can take it.

---------

Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
2026-10-31 19:37:15 +01:00
BruebachL
825aba953a
[Servatrice] Fail loudly when startup initialization fails (#7602)
* [Servatrice] Exit non-zero when server initialization fails

initServer() reported every startup failure through a zero exit code: the
database could not be opened, the server printed nothing of its own, and the
process looked like a clean shutdown. Log the two refusal reasons at critical
level and return 1 so supervisors and the Docker exit status can tell the
difference.

* [Servatrice] Keep console logging independent of the logfile

logMessage() returned before the console echo unless a logfile was open and
server/writelog was true, so --log-to-console produced nothing when
server/logfile was empty or could not be opened. That is how a refused database
connection could end in a silent exit.

Print to the console before the logfile gates, drop the duplicate echo from
flushBuffer(), and read writelog/logfilters once in main instead of calling the
non-thread-safe global QSettings from every logging thread.

* [Servatrice] Uninstall the log handler before tearing down globals

The handler reaches into the global logger and settingsCache. Leaving it
installed while main deletes those objects turns any Qt warning emitted during
QCoreApplication/QFactoryLoader teardown into a use-after-free; with
QT_DEBUG_PLUGINS=1 that warning exists, and Servatrice died with SIGSEGV in
QSettings::value() on the way out.

* [Servatrice] Buffer console logs on the logger thread

Address review feedback on the startup failure change:

- Append console lines to a buffer drained by the logger thread, so a
  stalled stdout reader cannot block game threads.
- Report islServer->errorString() instead of the literal call text.
- Let the constructor reuse reloadConfig() instead of parsing the filter
  list twice.

---------

Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
2026-10-31 19:02:49 +01:00