[CardDB] Break the CardInfo <-> CardSet cycle on reload (#7645)

* [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>

libcockatrice_card/libcockatrice/card/card_info.cpp

@@ -6,6 +6,7 @@
 #include "relation/card_relation.h"
 #include "set/card_set.h"
 
+#include <QDebug>
 #include <QHashIterator>
 #include <QRegularExpression>
 #include <QSharedPointer>
@@ -19,6 +20,18 @@ const QHash<QString, QString> &CardInfo::getPropertiesHash() const
     return properties.getProperties();
 }
 
+void CardInfo::notifyChanged()
+{
+    const CardInfoPtr self = sharedFromThis();
+    Q_ASSERT_X(self, "CardInfo::notifyChanged", "every CardInfo is owned by a QSharedPointer");
+    if (!self) {
+        qCWarning(CardInfoLog) << "notifyChanged() on an unowned CardInfo" << name << "- observers stay stale";
+        return;
+    }
+
+    emit cardInfoChanged(self);
+}
+
 void CardInfo::setProperty(const QString &_name, const QString &_value)
 {
     bool changed = properties.insert(_name, _value);
@@ -26,7 +39,7 @@ void CardInfo::setProperty(const QString &_name, const QString &_value)
         return;
     }
 
-    emit cardInfoChanged(smartThis);
+    notifyChanged();
 }
 
 CardInfo::CardInfo(const QString &_name,
@@ -97,7 +110,6 @@ CardInfoPtr CardInfo::newInstance(const QString &_name,
 {
     CardInfoPtr ptr(new CardInfo(_name, _text, _isToken, _properties, _relatedCards, _reverseRelatedCards, _sets,
                                  _uiAttributes, std::move(_localizedNames), std::move(_localizedTexts)));
-    ptr->setSmartPointer(ptr);
 
     for (const auto &printings : _sets) {
         for (const PrintingInfo &printing : printings) {
@@ -126,7 +138,6 @@ CardInfoPtr CardInfo::newInstance(const QString &_name,
     CardInfoPtr ptr(new CardInfo(_name, _text, _isToken, std::move(_propertiesBlob), _relatedCards,
                                  _reverseRelatedCards, _sets, _uiAttributes, std::move(_simpleName),
                                  std::move(_altNames), std::move(_localizedNames), std::move(_localizedTexts)));
-    ptr->setSmartPointer(ptr);
 
     if (_appendToSets) {
         for (const auto &printings : _sets) {
@@ -169,8 +180,15 @@ bool CardInfo::isLegalInFormat(const QString &format) const
 
 void CardInfo::addToSet(const CardSetPtr &_set, const PrintingInfo &_info)
 {
-    if (!_set->contains(smartThis)) {
+    const CardInfoPtr self = sharedFromThis();
-        _set->append(smartThis);
+    Q_ASSERT_X(self, "CardInfo::addToSet", "every CardInfo is owned by a QSharedPointer");
+    if (!self) {
+        qCWarning(CardInfoLog) << "addToSet() on an unowned CardInfo" << name << "- the set will not list it";
+        return;
+    }
+
+    if (!_set->contains(self)) {
+        _set->append(self);
     }
     if (!setsToPrintings[_set->getShortName()].contains(_info)) {
         setsToPrintings[_set->getShortName()].append(_info);
@@ -188,7 +206,7 @@ void CardInfo::combineLegalities(const QHash<QString, QString> &props)
             properties.insert(it.key(), it.value());
         }
     }
-    emit cardInfoChanged(smartThis);
+    notifyChanged();
 }
 
 void CardInfo::refreshCachedSets()

libcockatrice_card/libcockatrice/card/card_info.h

@@ -7,6 +7,7 @@
 
 #include <QByteArray>
 #include <QChar>
+#include <QEnableSharedFromThis>
 #include <QHash>
 #include <QList>
 #include <QLoggingCategory>
@@ -50,7 +51,7 @@ Q_DECLARE_METATYPE(CardInfoPtr)
  * Each CardInfo may belong to multiple sets through its printings, and can
  * be related to other cards through defined relationships.
  */
-class CardInfo : public QObject
+class CardInfo : public QObject, public QEnableSharedFromThis<CardInfo>
 {
     Q_OBJECT
 
@@ -74,11 +75,10 @@ private:
      *  @anchor PrivateCardProperties
      */
     ///@{
-    CardInfoPtr smartThis; ///< Smart pointer to self for safe cross-references.
+    QString name;       ///< Full name of the card.
-    QString name;          ///< Full name of the card.
+    QString simpleName; ///< Simplified name for fuzzy matching.
-    QString simpleName;    ///< Simplified name for fuzzy matching.
+    QString text;       ///< Text description or rules text of the card.
-    QString text;          ///< Text description or rules text of the card.
+    bool isToken;       ///< Whether this card is a token or not.
-    bool isToken;          ///< Whether this card is a token or not.
 
     QMap<QString, QString> localizedNames; ///< Localized card names, keyed by language code.
     QMap<QString, QString> localizedTexts; ///< Localized rules text, keyed by language code.
@@ -162,13 +162,14 @@ public:
      * @param other Another CardInfo to copy.
      */
     CardInfo(const CardInfo &other)
-        : QObject(other.parent()), name(other.name), simpleName(other.simpleName), text(other.text),
+        : QObject(other.parent()), QEnableSharedFromThis<CardInfo>(), name(other.name), simpleName(other.simpleName),
-          isToken(other.isToken), localizedNames(other.localizedNames), localizedTexts(other.localizedTexts),
+          text(other.text), isToken(other.isToken), localizedNames(other.localizedNames),
-          properties(other.properties), relatedCards(other.relatedCards),
+          localizedTexts(other.localizedTexts), properties(other.properties), relatedCards(other.relatedCards),
           reverseRelatedCards(other.reverseRelatedCards), reverseRelatedCardsToMe(other.reverseRelatedCardsToMe),
           setsToPrintings(other.setsToPrintings), uiAttributes(other.uiAttributes), setsNames(other.setsNames),
           altNames(other.altNames)
     {
+        // The copy deliberately starts unowned: the QSharedPointer that wraps it registers the self reference.
     }
 
     /**
@@ -252,27 +253,13 @@ public:
     /**
      * @brief Clones the current CardInfo instance.
      *
-     * Uses the copy constructor and ensures the smart pointer is properly set.
+     * Uses the copy constructor; the copy is managed by its own shared pointer.
      *
      * @return Shared pointer to the cloned CardInfo.
      */
     [[nodiscard]] CardInfoPtr clone() const
     {
-        auto newCardInfo = CardInfoPtr(new CardInfo(*this));
+        return CardInfoPtr(new CardInfo(*this));
-        newCardInfo->setSmartPointer(newCardInfo); // Set the smart pointer for the new instance
-        return newCardInfo;
-    }
-
-    /**
-     * @brief Sets the internal smart pointer to self.
-     *
-     * Used internally to allow safe cross-references among CardInfo and CardSet.
-     *
-     * @param _ptr Shared pointer pointing to this instance.
-     */
-    void setSmartPointer(CardInfoPtr _ptr)
-    {
-        smartThis = std::move(_ptr);
     }
 
     /** @name Basic Properties Accessors */ //@{
@@ -295,7 +282,7 @@ public:
     void setText(const QString &_text)
     {
         text = _text;
-        emit cardInfoChanged(smartThis);
+        notifyChanged();
     }
 
     /**
@@ -358,7 +345,7 @@ public:
             return;
         }
         localizedNames.insert(lang, _localizedName);
-        emit cardInfoChanged(smartThis);
+        notifyChanged();
     }
 
     /**
@@ -373,7 +360,7 @@ public:
             return;
         }
         localizedTexts.insert(lang, _localizedText);
-        emit cardInfoChanged(smartThis);
+        notifyChanged();
     }
 
     /**
@@ -552,6 +539,14 @@ public:
     static QString simplifyName(const QString &name);
 
 private:
+    /**
+     * @brief Emits cardInfoChanged() with a strong reference to this card.
+     *
+     * A CardInfo is always owned by a QSharedPointer, so the self reference is always available; a released
+     * build that somehow finds it missing warns rather than emitting a null card.
+     */
+    void notifyChanged();
+
     /**
      * @brief Refreshes the cached, human-readable list of set names.
      *

libcockatrice_card/libcockatrice/card/database/card_database.cpp

@@ -53,13 +53,26 @@ CardDatabase::~CardDatabase()
     clear();
 }
 
+void CardDatabase::detachSetMembership()
+{
+    for (const auto &set : sets) {
+        if (set) {
+            set->clear();
+        }
+    }
+}
+
 void CardDatabase::clear()
 {
     QMutexLocker locker(clearDatabaseMutex);
 
+    // The sets hold strong card references, so empty them before dropping the cards: this breaks the
+    // CardInfo <-> CardSet cycle and spares the per-card detach over the whole database.
+    detachSetMembership();
+
     for (const auto &card : cards.values()) {
         if (card) {
-            removeCard(card);
+            unindexCard(card);
         }
     }
 
@@ -156,6 +169,23 @@ void CardDatabase::removeCard(const CardInfoPtr &card)
         return;
     }
 
+    // The set holds a strong reference back to the card, so detach it: leaving it there keeps both
+    // objects alive after the database drops the card. newInstance() appends through the first
+    // printing of every set, so detaching through that same printing mirrors the append.
+    for (const auto &printings : card->getSets()) {
+        if (printings.isEmpty()) {
+            continue;
+        }
+        if (const CardSetPtr set = printings.first().getSet()) {
+            set->removeAll(card);
+        }
+    }
+
+    unindexCard(card);
+}
+
+void CardDatabase::unindexCard(const CardInfoPtr &card)
+{
     for (auto *cardRelation : card->getRelatedCards()) {
         cardRelation->deleteLater();
     }
@@ -280,6 +310,10 @@ void CardDatabase::swapInDatabaseData(CardDatabaseData data)
         card->clearDirectRelations();
     }
 
+    // Break the CardSet <-> CardInfo cycle before replacing the containers, otherwise the previous
+    // snapshot survives the reload through the sets' strong card lists.
+    detachSetMembership();
+
     cards = std::move(data.cards);
     simpleNameCards = std::move(data.simpleNameCards);
     sets = std::move(data.sets);

libcockatrice_card/libcockatrice/card/database/card_database.h

@@ -74,6 +74,23 @@ private:
      */
     void refreshCachedReverseRelatedCards(CardNameMap &cardMap);
 
+    /**
+     * @brief Empties every set, dropping the strong card references they hold.
+     *
+     * A card keeps a CardSetPtr through its printings while its set keeps a strong CardInfoPtr, so the
+     * two form a reference cycle. Dropping a whole snapshot has to break that cycle once, up front,
+     * instead of relying on the per-card detach in removeCard().
+     */
+    void detachSetMembership();
+
+    /**
+     * @brief Drops a card from the database indexes and notifies observers.
+     *
+     * Callers are responsible for detaching the card from its sets first.
+     * @param card Pointer to the card to unindex.
+     */
+    void unindexCard(const CardInfoPtr &card);
+
     /** @brief Mutexes for thread safety. */
     QBasicMutex *clearDatabaseMutex = new QBasicMutex(), *addCardMutex = new QBasicMutex(),
                 *removeCardMutex = new QBasicMutex();

libcockatrice_card/libcockatrice/card/set/card_set.cpp

@@ -27,7 +27,6 @@ CardSetPtr CardSet::newInstance(ICardSetPriorityController *_priorityController,
                                 const Priority _priority)
 {
     CardSetPtr ptr(new CardSet(_priorityController, _shortName, _longName, _setType, _releaseDate, _priority));
-    // ptr->setSmartPointer(ptr);
     return ptr;
 }
 

servatrice/src/server_logger.cpp

@@ -6,6 +6,7 @@
 #include <QFileInfo>
 #include <QForeach>
 #include <QIODevice>
+#include <QMutexLocker>
 #include <QTextStream>
 #include <QThread>
 #include <QtGlobal>
@@ -66,7 +67,9 @@ void ServerLogger::reloadConfig(bool _writeLog, const QString &_logFilters)
 
 bool ServerLogger::isLoggingEnabled() const
 {
-    return logFile != nullptr && settingsCache->value("server/writelog", 1).toBool();
+    // Mirror logMessage(): the console is an independent sink and the logfile needs writeLog.
+    QMutexLocker locker(&configMutex);
+    return logToConsole || (logFile != nullptr && writeLog);
 }
 
 void ServerLogger::logMessage(const QString &message, void *caller)

servatrice/src/server_logger.h

@@ -41,7 +41,7 @@ private:
     bool messageFilterActive = false;
     QStringList logFilters;
     // Guards writeLog/messageFilterActive/logFilters: SIGHUP rewrites them while other threads log.
-    QMutex configMutex;
+    mutable QMutex configMutex;
     QFile *logFile = nullptr;
     bool flushRunning;
     QStringList buffer;

tests/carddatabase/carddatabase_test.cpp

@@ -17,10 +17,12 @@
 #include <QIODevice>
 #include <QList>
 #include <QSharedPointer>
+#include <QSignalSpy>
 #include <QString>
 #include <QStringList>
 #include <QTemporaryDir>
 #include <QVector>
+#include <QWeakPointer>
 #include <libcockatrice/card/card_info.h>
 #include <libcockatrice/card/database/card_database_data.h>
 #include <libcockatrice/card/database/parser/cockatrice_xml_4.h>
@@ -608,6 +610,110 @@ TEST_F(CardDatabaseCacheInvalidationTest, MarkAllSetsAsKnownIsBatched)
     // Ignored sets keep their known state but are enabled.
     EXPECT_TRUE(controller.isEnabled("XXX"));
 }
+
+TEST_F(CardDatabaseCacheInvalidationTest, ReloadReleasesPreviousSnapshot)
+{
+    NoopCardPreferenceProvider prefs;
+    SandboxPathProvider pathProvider(root);
+    CardDatabase db(nullptr, &prefs, &pathProvider, &controller);
+
+    db.loadCardDatabases();
+    CardInfoPtr card = db.getCardList().value("Cat");
+    ASSERT_FALSE(card.isNull());
+    QWeakPointer<CardInfo> weakCard(card);
+    card.clear();
+
+    db.reloadCardDatabasesAndNotify();
+
+    // CardSet holds strong CardInfoPtrs and CardInfo holds the CardSetPtr through its printings; if that
+    // cycle is not broken, the previous snapshot survives the reload.
+    EXPECT_TRUE(weakCard.isNull()) << "a reload must release the previous card snapshot";
+}
+
+TEST_F(CardDatabaseCacheInvalidationTest, ClearReleasesCards)
+{
+    NoopCardPreferenceProvider prefs;
+    SandboxPathProvider pathProvider(root);
+    CardDatabase db(nullptr, &prefs, &pathProvider, &controller);
+
+    db.loadCardDatabases();
+    CardInfoPtr card = db.getCardList().value("Cat");
+    ASSERT_FALSE(card.isNull());
+    QWeakPointer<CardInfo> weakCard(card);
+    card.clear();
+
+    db.clear();
+
+    EXPECT_TRUE(weakCard.isNull()) << "clear() must release cards held by set membership";
+}
+
+TEST_F(CardDatabaseCacheInvalidationTest, RemoveCardDetachesItFromItsSet)
+{
+    // removeCard() is the one teardown path a caller can trigger for a single card (the token editor
+    // uses it). Without the detach the set's strong reference keeps the removed card alive forever.
+    NoopCardPreferenceProvider prefs;
+    SandboxPathProvider pathProvider(root);
+    CardDatabase db(nullptr, &prefs, &pathProvider, &controller);
+
+    db.loadCardDatabases();
+    CardInfoPtr card = db.getCardList().value("Cat");
+    ASSERT_FALSE(card.isNull());
+    const CardSetPtr catSet = db.getSet("CAT");
+    ASSERT_TRUE(catSet->contains(card));
+
+    db.removeCard(card);
+
+    EXPECT_FALSE(catSet->contains(card)) << "removeCard() must detach the card from its set";
+    QWeakPointer<CardInfo> weakCard(card);
+    card.clear();
+    EXPECT_TRUE(weakCard.isNull()) << "a removed card must not stay alive through its set";
+}
+
+TEST_F(CardDatabaseCacheInvalidationTest, RemoveSelfPointerStillNotifiesDatabaseCard)
+{
+    // cardInfoChanged() used to be emitted through a self pointer stored on the card. Without it the
+    // card must still notify its observers whenever its properties change.
+    NoopCardPreferenceProvider prefs;
+    SandboxPathProvider pathProvider(root);
+    CardDatabase db(nullptr, &prefs, &pathProvider, &controller);
+
+    db.loadCardDatabases();
+    const CardInfoPtr card = db.getCardList().value("Cat");
+    ASSERT_FALSE(card.isNull());
+    QSignalSpy spy(card.data(), &CardInfo::cardInfoChanged);
+
+    card->setProperty("manacost", "1G");
+
+    ASSERT_EQ(spy.count(), 1) << "a property change on a database card must still emit cardInfoChanged";
+    const QList<QVariant> arguments = spy.takeFirst();
+    ASSERT_EQ(arguments.size(), 1);
+    EXPECT_EQ(arguments.first().value<CardInfoPtr>().data(), card.data())
+        << "the signal must carry the card that changed";
+}
+
+TEST_F(CardDatabaseCacheInvalidationTest, RemoveSelfPointerCloneNotifiesAsItself)
+{
+    // A clone owns its own shared reference, so its changes must be reported as the clone's, never the
+    // original's: decks and views listening to the original must not be invalidated by a clone edit.
+    NoopCardPreferenceProvider prefs;
+    SandboxPathProvider pathProvider(root);
+    CardDatabase db(nullptr, &prefs, &pathProvider, &controller);
+
+    db.loadCardDatabases();
+    const CardInfoPtr original = db.getCardList().value("Cat");
+    ASSERT_FALSE(original.isNull());
+    const CardInfoPtr clone = original->clone();
+    QSignalSpy originalSpy(original.data(), &CardInfo::cardInfoChanged);
+    QSignalSpy cloneSpy(clone.data(), &CardInfo::cardInfoChanged);
+
+    clone->setProperty("manacost", "1G");
+
+    ASSERT_EQ(cloneSpy.count(), 1) << "the clone must notify its own observers";
+    EXPECT_EQ(cloneSpy.first().first().value<CardInfoPtr>().data(), clone.data())
+        << "the clone's signal must carry the clone";
+    EXPECT_EQ(originalSpy.count(), 0) << "the clone must not notify the original's observers";
+}
+
 } // namespace
 
 int main(int argc, char **argv)

tests/server_logger_test.cpp

@@ -98,6 +98,28 @@ TEST(ServerLoggerTest, StaysQuietWhenConsoleIsDisabled)
     EXPECT_TRUE(capture.text().isEmpty()) << capture.text().toStdString();
 }
 
+TEST(ServerLoggerTest, IsLoggingEnabledMirrorsTheSinks)
+{
+    QTemporaryDir dir;
+    ASSERT_TRUE(dir.isValid());
+
+    // The console is a sink of its own: messages must not be discarded for want of a logfile.
+    {
+        ServerLogger logger(true, false, QString());
+        EXPECT_TRUE(logger.isLoggingEnabled());
+    }
+
+    // The logfile sink needs an open file and writelog; reloadConfig() carries the SIGHUP change.
+    {
+        ServerLogger logger(false, true, QString());
+        EXPECT_FALSE(logger.isLoggingEnabled()) << "no logfile is open yet";
+        logger.startLog(dir.filePath("enabled.log"));
+        EXPECT_TRUE(logger.isLoggingEnabled());
+        logger.reloadConfig(false, QString());
+        EXPECT_FALSE(logger.isLoggingEnabled()) << "writelog=false leaves no sink enabled";
+    }
+}
+
 TEST(ServerLoggerTest, WritesMatchingFilteredMessagesToTheLogFile)
 {
     QTemporaryDir dir;