Revert "fix(reorder): run reorder synchronously on UI thread to fix CI race"

tamlok · tamlok · commit 8f9d3c249228 · 2026-06-18T02:51:09.000+08:00
This reverts commit b331214.
diff --git a/src/core/services/notebookcoreservice.cpp b/src/core/services/notebookcoreservice.cpp
@@ -4,6 +4,7 @@
 #include <QJsonParseError>
 #include <QMetaObject>
 #include <QVariantMap>
+#include <QtConcurrent>
 
 #include <nlohmann/json.hpp>
 
@@ -812,36 +813,32 @@ QJsonObject NotebookCoreService::listFolderChildren(const QString &p_notebookId,
   return parseJsonObjectFromCStr(json);
 }
 
-// T6 (notebook-explorer-drag-reorder): reorder with hooks.
+// T6 (notebook-explorer-drag-reorder): async reorder with hooks + IoGate.
 //
-// Threading layout — ALL vxcore work runs SYNCHRONOUSLY on the caller (GUI)
-// thread, exactly like every sibling folder mutation (createFolder /
-// createFile / rename / delete). This is mandatory for correctness: vxcore's
-// FolderManager config cache is NOT thread-safe and the model reads
-// (NotebookNodeModel::fetchMore -> listFolderChildren) run on the GUI thread,
-// so the reorder's read-modify-write MUST be one atomic GUI-thread unit with
-// respect to those reads. An earlier design dispatched the write to a
-// QtConcurrent worker that only held NotebookIoGate while marshalling the
-// vxcore call back to the GUI thread; that left the snapshot read and the
-// write as two separate GUI-thread events, with the QSignalSpy::wait nested
-// event loop and queued model/folder events free to interleave between them —
-// the source of a deterministic VXCORE_ERR_PERMUTATION_MISMATCH on 2-core CI.
-//
-//   1. Pre-validate args.
-//   2. Snapshot current order via listFolderChildren.
-//   3. No-op short-circuit (no hooks, no write).
-//   4. Fire before_reorder synchronously (gives plugins a cancel slot
-//      BEFORE any disk work happens).
-//   5. Perform the vxcore_folder_set_children_order write synchronously.
-//   6. Deliver completion (after-hook + reorderCompleted signal) ASYNCHRONOUSLY
-//      via Qt::QueuedConnection, preserving the public async contract for
-//      callers/tests that wait for the signal after this function returns.
+// Threading layout:
+//   Caller thread (typically GUI):
+//     1. Pre-validate args.
+//     2. Snapshot current order via listFolderChildren.
+//     3. No-op short-circuit (no hooks, no worker).
+//     4. Fire before_reorder synchronously (gives plugins a cancel slot
+//        BEFORE any disk work is queued).
+//     5. Dispatch to QtConcurrent worker; return.
+//   Worker thread:
+//     6. Hold NotebookIoGate::ScopedLock(notebookId) for the duration of
+//        the vxcore_folder_set_children_order call ONLY.
+//     7. Release the gate by leaving the inner { } scope (RAII).
+//     8. Queue completion (after-hook + reorderCompleted signal) onto the
+//        service's owning thread via QMetaObject::invokeMethod /
+//        Qt::QueuedConnection. Releasing the gate BEFORE queuing the
+//        after-hook is what makes it safe for plugins to re-enter the gate
+//        from inside the after-hook callback (e.g., to stage a follow-up
+//        write) without deadlocking.
 void NotebookCoreService::reorderFolderChildren(const QString &p_notebookId,
                                                 const QString &p_folderRelPath,
                                                 const QStringList &p_orderedFolders,
                                                 const QStringList &p_orderedFiles) {
   if (p_notebookId.isEmpty() || (p_orderedFolders.isEmpty() && p_orderedFiles.isEmpty()) ||
-      !m_hookMgr) {
+      !m_hookMgr || !m_ioGate) {
     emit reorderCompleted(p_notebookId, p_folderRelPath, false, tr("Invalid arguments"));
     return;
   }
@@ -884,48 +881,58 @@ void NotebookCoreService::reorderFolderChildren(const QString &p_notebookId,
     return;
   }
 
-  // THREADING (CI race fix): the snapshot read above (listFolderChildren),
-  // this write, and every model read (NotebookNodeModel::fetchMore ->
-  // listFolderChildren) all hit vxcore's FolderManager, whose config cache is
-  // NOT thread-safe. A correct reorder needs the read-modify-write to be a
-  // single ATOMIC unit with respect to those reads. The previous design ran a
-  // QtConcurrent worker that merely held NotebookIoGate while marshalling the
-  // sole vxcore call back to the UI thread via BlockingQueuedConnection — so
-  // the gate window and the actual vxcore-access window were disjoint, and the
-  // snapshot read (line above) and the write became two separate UI-thread
-  // events. The QSignalSpy::wait()/nested event loop and queued model+folder
-  // event delivery could run BETWEEN them, letting the write observe an
-  // inconsistent root config (current=0) -> VXCORE_ERR_PERMUTATION_MISMATCH on
-  // the 2-core CI scheduler.
-  //
-  // Fix: run the vxcore write SYNCHRONOUSLY on the calling (UI) thread, exactly
-  // like every sibling NotebookCoreService folder mutation (createFolder,
-  // createFile, rename, delete) already does. With no worker and no nested
-  // event loop between the snapshot and the write, the cache the write sees is
-  // the cache the snapshot saw. The completion is still delivered
-  // ASYNCHRONOUSLY (Qt::QueuedConnection) so the public reorderCompleted
-  // contract — and callers/tests that wait for the signal AFTER this returns —
-  // are unchanged.
+  // Capture by value so the worker has stable copies (Qt strings are CoW so
+  // this is cheap). Capturing `this` is safe — the service outlives all in-
+  // flight reorders by construction (it is destroyed only at shutdown after
+  // QThreadPool::waitForDone via ~QtConcurrent::run cleanup).
   const QString notebookId = p_notebookId;
   const QString folderRelPath = p_folderRelPath;
-  const std::string orderedJson = buildOrderedJson(p_orderedFolders, p_orderedFiles);
-  const int rc = static_cast<int>(vxcore_folder_set_children_order(
-      m_context, notebookId.toUtf8().constData(),
-      folderRelPath.isEmpty() ? "." : folderRelPath.toUtf8().constData(), orderedJson.c_str()));
-
-  QMetaObject::invokeMethod(
-      this,
-      [this, notebookId, folderRelPath, event, rc]() {
-        if (rc == VXCORE_OK) {
-          if (m_hookMgr) {
-            m_hookMgr->doAction(HookNames::NodeAfterReorder, event);
+  const QStringList orderedFolders = p_orderedFolders;
+  const QStringList orderedFiles = p_orderedFiles;
+
+  QtConcurrent::run([this, notebookId, folderRelPath, orderedFolders, orderedFiles, event]() {
+    int rc = VXCORE_ERR_UNKNOWN;
+    {
+      // Acquire-release window: hold the gate for the working-tree write ONLY.
+      // Mirrors the SyncOps::triggerSync stage-phase pattern.
+      NotebookIoGate::ScopedLock lock(*m_ioGate, notebookId);
+      const std::string orderedJson = buildOrderedJson(orderedFolders, orderedFiles);
+      // THREADING: vxcore's FolderManager is NOT thread-safe — its folder-config
+      // cache is shared with main-thread reads (NotebookNodeModel::fetchMore ->
+      // listFolderChildren). Run the actual vxcore folder write on the MAIN
+      // thread so it is serialized with all other main-thread vxcore folder
+      // access; otherwise a concurrent main-thread cache reload can free the
+      // cached FolderConfig under the worker, yielding the current=0
+      // PERMUTATION_MISMATCH that made this reorder spuriously fail. The
+      // NotebookIoGate stays held HERE on the worker (async acquisition that
+      // never blocks the UI thread) to keep serializing the working-tree write
+      // against save/sync workers; BlockingQueuedConnection keeps the gate held
+      // across the write without releasing it early.
+      QMetaObject::invokeMethod(
+          this,
+          [this, &rc, &notebookId, &folderRelPath, &orderedJson]() {
+            rc = static_cast<int>(vxcore_folder_set_children_order(
+                m_context, notebookId.toUtf8().constData(),
+                folderRelPath.isEmpty() ? "." : folderRelPath.toUtf8().constData(),
+                orderedJson.c_str()));
+          },
+          Qt::BlockingQueuedConnection);
+    } // Gate released here BEFORE queuing the after-hook.
+
+    QMetaObject::invokeMethod(
+        this,
+        [this, notebookId, folderRelPath, event, rc]() {
+          if (rc == VXCORE_OK) {
+            if (m_hookMgr) {
+              m_hookMgr->doAction(HookNames::NodeAfterReorder, event);
+            }
+            emit reorderCompleted(notebookId, folderRelPath, true, QString());
+          } else {
+            emit reorderCompleted(notebookId, folderRelPath, false, reorderErrorToString(rc));
           }
-          emit reorderCompleted(notebookId, folderRelPath, true, QString());
-        } else {
-          emit reorderCompleted(notebookId, folderRelPath, false, reorderErrorToString(rc));
-        }
-      },
-      Qt::QueuedConnection);
+        },
+        Qt::QueuedConnection);
+  });
 }
 
 std::string NotebookCoreService::buildOrderedJson(const QStringList &p_orderedFolders,
diff --git a/src/core/services/notebookcoreservice.h b/src/core/services/notebookcoreservice.h
@@ -37,12 +37,11 @@ class NotebookCoreService : public QObject, public ISyncNotebookService, private
   // Called from main() after both services are constructed.
   void setHookManager(HookManager *p_hookMgr);
 
-  // Set NotebookIoGate (the per-notebook working-tree serializer shared with
-  // BufferSaveQueue / SyncOps). Retained for dependency wiring; NOT consulted by
-  // reorderFolderChildren, which — like its sibling folder mutations
-  // (createFolder/createFile/rename/delete) — now runs synchronously on the UI
-  // thread (see reorderFolderChildren in the .cpp for the rationale). Called
-  // from main() with the SAME instance injected into the other actors.
+  // Set NotebookIoGate for serializing reorder writes against save / sync work
+  // on the same notebook's working tree. T6: required by reorderFolderChildren.
+  // Called from main() with the SAME instance already injected into
+  // BufferSaveQueue / SyncOps — sharing the gate is what makes per-notebook
+  // serialization actually serialize across actors.
   void setNotebookIoGate(NotebookIoGate *p_ioGate);
 
   // Notebook operations (7 methods).
@@ -201,19 +200,17 @@ class NotebookCoreService : public QObject, public ISyncNotebookService, private
   QJsonObject listFolderChildren(const QString &p_notebookId, const QString &p_folderPath) const;
 
   // Atomically rewrite the order of a folder's children (subfolders / files)
-  // in its vx.json. The vxcore write runs SYNCHRONOUSLY on the calling (UI)
-  // thread — like every sibling folder mutation — because vxcore's FolderManager
-  // cache is not thread-safe and is read on the UI thread (model fetchMore);
-  // the read-modify-write must therefore be one atomic UI-thread step. The
-  // result is delivered ASYNCHRONOUSLY via the reorderCompleted signal
-  // (Qt::QueuedConnection), so callers may wait() for it after this returns.
+  // in its vx.json. Returns immediately; the actual disk write happens on a
+  // worker thread holding NotebookIoGate::ScopedLock(notebookId), and the
+  // result is delivered via the reorderCompleted signal.
   //
   // Hook contract:
   //   - vnote.node.before_reorder fires SYNCHRONOUSLY on the calling thread.
   //     A hook that calls ctx.cancel() short-circuits with success=false
   //     and errorMessage="Cancelled by hook"; the vxcore call is NOT made.
   //   - vnote.node.after_reorder fires on this service's owning thread AFTER
-  //     a successful write, just before reorderCompleted(true).
+  //     the worker has released the IoGate, so handlers can safely re-enter
+  //     the gate without deadlock risk.
   //
   // No-op contract:
   //   If both presented sub-arrays match (or are empty), the method emits
diff --git a/tests/core/test_notebook_core_service_reorder.cpp b/tests/core/test_notebook_core_service_reorder.cpp
@@ -46,7 +46,7 @@ private slots:
   void testNoOpSkipsHooksAndVxcore();
   void testVxcoreErrorSkipsAfterHookAndReportsFailure();
   void testHookCancelSkipsVxcoreAndReportsCancelled();
-  void testReorderIsIndependentOfIoGate();
+  void testIoGateSerializesWithExternalHolder();
   void testAfterHookFiresAfterLockRelease();
 
 private:
@@ -369,34 +369,16 @@ void TestNotebookCoreServiceReorder::testHookCancelSkipsVxcoreAndReportsCancelle
   m_hookMgr->removeAction(afterId);
 }
 
-// ===== 5. Reorder is INDEPENDENT of the NotebookIoGate =====
-//
-// CONTRACT CHANGE (CI race fix): reorderFolderChildren now runs the vxcore
-// write SYNCHRONOUSLY on the calling (UI) thread, exactly like its sibling
-// folder mutations (createFolder/createFile/rename/delete). It deliberately
-// does NOT acquire the per-notebook NotebookIoGate. Rationale: vxcore's
-// FolderManager cache is not thread-safe and is read on the UI thread (model
-// fetchMore -> listFolderChildren); the only correct serialization is
-// UI-thread atomicity. A gate held on a worker thread (the retired design)
-// could never serialize against those UI-thread reads — it guarded the wrong
-// resource on the wrong thread and produced a deterministic PERMUTATION_MISMATCH
-// on 2-core CI runners.
-//
-// This test pins the new contract: a foreign thread holding the gate does NOT
-// block reorder. (The acknowledged, pre-existing flip-side — a structural
-// vx.json write can race a concurrent git-stage, exactly as the ungated
-// siblings already can — is tracked as a future "serialize all FolderManager
-// writes" refactor. Do NOT "fix" this test back to gate-serialization.)
-void TestNotebookCoreServiceReorder::testReorderIsIndependentOfIoGate() {
+// ===== 5. IoGate serialization =====
+void TestNotebookCoreServiceReorder::testIoGateSerializesWithExternalHolder() {
   QStringList seedFolders, seedFiles;
   const QString nbId = seedNotebook(QStringLiteral("nb_iogate"), &seedFolders, &seedFiles);
   QVERIFY(!nbId.isEmpty());
 
   QStringList newFolders = seedFolders;
   std::reverse(newFolders.begin(), newFolders.end());
 
-  // Acquire the gate from a foreign thread BEFORE invoking reorder and hold it
-  // for the entire reorder. Reorder must NOT depend on it.
+  // Acquire the gate from a foreign thread BEFORE invoking reorder.
   std::atomic<bool> holderHasLock{false};
   std::atomic<bool> releaseRequested{false};
   QThread *holder = QThread::create([&]() {
@@ -414,17 +396,24 @@ void TestNotebookCoreServiceReorder::testReorderIsIndependentOfIoGate() {
   QSignalSpy spy(m_service, &NotebookCoreService::reorderCompleted);
   m_service->reorderFolderChildren(nbId, QString(), newFolders, QStringList());
 
-  // Reorder completes (and persists) even while the external holder owns the
-  // gate — proving the gate is not on reorder's path.
-  QVERIFY(spy.wait(5000));
-  QCOMPARE(spy.size(), 1);
-  QCOMPARE(spy.takeFirst().at(2).toBool(), true);
-  QCOMPARE(listFolders(nbId), newFolders);
+  // While the external holder owns the gate, reorder must NOT progress.
+  bool finishedEarly = spy.wait(400);
+  QVERIFY2(!finishedEarly, "reorder completed while external IoGate holder still owned the lock");
+  QCOMPARE(spy.size(), 0);
 
-  // Release the foreign holder.
+  // Release the gate.
   releaseRequested.store(true);
   holder->wait();
   delete holder;
+
+  // Now reorder should drain.
+  if (spy.isEmpty()) {
+    QVERIFY(spy.wait(5000));
+  }
+  QCOMPARE(spy.size(), 1);
+  QCOMPARE(spy.takeFirst().at(2).toBool(), true);
+
+  QCOMPARE(listFolders(nbId), newFolders);
 }
 
 // ===== 6. After-hook fires AFTER lock release =====
diff --git a/tests/integration/test_reorder_edge_cases.cpp b/tests/integration/test_reorder_edge_cases.cpp
@@ -914,22 +914,13 @@ void TestReorderEdgeCases::EC11_performance100Children() {
 }
 
 // =============================================================================
-// EC12: Reorder runs concurrently with a save (gate-independent)
+// EC12: Concurrent save + reorder serialization
 // =============================================================================
-// CONTRACT CHANGE (CI race fix): reorderFolderChildren now performs its vxcore
-// write SYNCHRONOUSLY on the calling (UI) thread — like every sibling folder
-// mutation (createFolder/createFile/rename/delete) — and deliberately does NOT
-// acquire the per-notebook NotebookIoGate. The gate could only ever be held on
-// a worker thread, which cannot serialize against the UI-thread FolderManager
-// reads (model fetchMore) that actually race the write; the retired gated
-// design produced a deterministic PERMUTATION_MISMATCH on 2-core CI.
-//
-// This test pins the new contract: a foreign thread holding the gate (a stand-in
-// for a concurrent buffer-save / sync-stage) does NOT block reorder; reorder
-// completes and persists regardless. (The acknowledged flip-side — a structural
-// vx.json write racing a concurrent git-stage, which the ungated siblings ALSO
-// permit — is tracked as a future "serialize all FolderManager writes" refactor.
-// Do NOT "fix" this back to gate-serialization.)
+// Hold the per-notebook IoGate from a worker thread for ~150ms, then trigger
+// reorder. The reorder MUST block until the external holder releases the
+// gate — proving the gate's per-notebook serialization contract still
+// protects the working tree from interleaved writes. Mirrors the IoGate
+// integration test in tests/core/test_notebook_core_service_reorder.cpp.
 void TestReorderEdgeCases::EC12_concurrentSaveAndReorder() {
   const QString nbId = seedNotebook(QStringLiteral("nb_ec12"),
                                     QStringList{QStringLiteral("sub_a"), QStringLiteral("sub_b")},
@@ -957,16 +948,22 @@ void TestReorderEdgeCases::EC12_concurrentSaveAndReorder() {
   QSignalSpy spy(m_service, &vnotex::NotebookCoreService::reorderCompleted);
   m_service->reorderFolderChildren(nbId, QString(), newFolders, QStringList());
 
-  // Reorder completes and persists even while the external holder owns the
-  // gate — the gate is not on reorder's path.
-  QVERIFY(spy.wait(5000));
-  QCOMPARE(spy.size(), 1);
-  QCOMPARE(spy.takeFirst().at(2).toBool(), true);
-  QCOMPARE(listFolders(nbId), newFolders);
+  // While the external holder owns the gate, the reorder worker must NOT
+  // complete. 400ms of polling proves the wait.
+  const bool finishedEarly = spy.wait(400);
+  QVERIFY2(!finishedEarly, "reorder completed while external IoGate holder still owned the lock");
+  QCOMPARE(spy.size(), 0);
 
   releaseRequested.store(true);
   holder->wait();
   delete holder;
+
+  if (spy.isEmpty()) {
+    QVERIFY(spy.wait(5000));
+  }
+  QCOMPARE(spy.size(), 1);
+  QCOMPARE(spy.takeFirst().at(2).toBool(), true);
+  QCOMPARE(listFolders(nbId), newFolders);
 }
 
 // =============================================================================
diff --git a/tests/widgets/test_two_columns_node_explorer_sort.cpp b/tests/widgets/test_two_columns_node_explorer_sort.cpp
@@ -264,12 +264,9 @@ void TestTwoColumnsNodeExplorerSort::
   explorer->requestReorderNodes(rootId, {QStringLiteral("beta"), QStringLiteral("alpha")},
                                 /*orderedFileNames=*/QStringList());
 
-  // nodesReordered is emitted via QueuedConnection after the synchronous
-  // reorder write. QTRY_COMPARE is order-robust: it passes whether the signal
-  // already arrived or arrives within the timeout, unlike QSignalSpy::wait
-  // which only catches a NEXT emission and spuriously fails if completion
-  // already landed before the wait was entered.
-  QTRY_COMPARE_WITH_TIMEOUT(reorderedSpy.count(), 1, 5000);
+  // nodesReordered is emitted via QueuedConnection from the reorder worker.
+  QVERIFY(reorderedSpy.wait(5000));
+  QCOMPARE(reorderedSpy.count(), 1);
   QCOMPARE(errorSpy.count(), 0);
 
   // Verify the emit carried the original parent identifier (notebookId
@@ -312,8 +309,7 @@ void TestTwoColumnsNodeExplorerSort::testRequestReorderNodesUsesSharedController
   explorer->requestReorderNodes(rootId, {QStringLiteral("beta"), QStringLiteral("alpha")},
                                 /*orderedFileNames=*/QStringList());
 
-  // Order-robust wait for the first service-level completion (see T12b note).
-  QTRY_VERIFY_WITH_TIMEOUT(serviceReorderedSpy.count() >= 1, 5000);
+  QVERIFY(serviceReorderedSpy.wait(5000));
 
   // Drain any pending events so a hypothetical double-dispatch would have
   // had a chance to surface as a second service emit. 100ms is generous
