Log whether a preserved-catalog attempt recorded, and tighten the lock and atomicity notes

Rhys Sullivan committed 2 months ago

Commit 2fb74bd1 parent e48fe133 files · +30 −7Browse files

Unified · Split

3 files

packages/core/sdk/src/executor.ts +22 −5
packages/core/sdk/src/plugin.ts +6 −1
packages/core/sdk/src/tools-sync-scope.test.ts +2 −1

         // column only if this exact value is still in it, so a mark written
         // while we were dialing carries a different token, survives the clear,
         // and re-lists the connection on the next read.
-        const observedStaleToken =
+        const observedStaleToken = existingRow?.tools_stale_token ?? null;
-          existingRow?.tools_stale_token == null ? null : String(existingRow.tools_stale_token);
         // Does this attempt still own the connection? A read-path refresh takes
         // a lease before dialing, and a slow one can lose it: another reader
          *  drift mark this listing answered — all three together, or none of
          *  them. Returns false when the attempt lost its lease.
          *
+         *  That atomicity is dialect-conditional: on D1
+         *  (`interactiveTransactions: false`) there is no transaction, the
+         *  statements auto-commit one by one, and an interrupted run can leave a
+         *  prefix of them applied. It still fails CLOSED, because the drift clear
+         *  is the LAST statement: any prefix short of the whole leaves the token
+         *  in place, so the connection stays drifted and re-lists.
+         *
          *  Already holds the persist permit, which is not reentrant: nothing in
          *  here may take it again.
          *
           // Reporting `incomplete` there would attribute a verdict to a row the
           // winning attempt owns.
           const recorded = yield* writeSyncOutcome(failureSet(kind, reason));
+          // `recorded` rides along because the warning is emitted either way: a
+          // lost lease preserved the catalog too, but wrote no ladder entry. Without
+          // the field, a rate derived from these logs would count attempts the
+          // `incomplete` counter never saw.
           yield* Effect.logWarning("executor tool sync preserved catalog", {
             reason,
             errorKind: kind ?? "unclassified",
             integration: String(ref.integration),
             connection: String(ref.name),
+            recorded,
           });
           return yield* persistedTools(recorded ? "incomplete" : "lost_claim");
         }
             // the plugin said nothing about why. Unclassified, so it backs off
             // but never parks.
             const recorded = yield* writeSyncOutcome(failureSet(null, reason));
+            // `recorded` for the same reason as above: the warning fires even when
+            // the lease was lost and nothing was written.
             yield* Effect.logWarning("executor tool sync preserved nonzero catalog", {
               reason,
               integration: String(ref.integration),
               connection: String(ref.name),
               existingToolCount: keptRows.length,
+              recorded,
             });
             // Re-read on a lost lease: `keptRows` predates the write, and the
             // attempt that owns the connection may have replaced the catalog
     // with a new one is precisely how this signal outlives the listing that
     // could not have seen it.
     //
-    // Locked: this is a lifecycle write like every other, and it can run while
+    // Locked: the permit serializes this write against the catalog-persist
-    // a read's refresh fan-out holds an open BEGIN on the shared SQLite
+    // transactions specifically — `replaceCatalog` and `writeSyncOutcome`, the
-    // connection — where a bare UPDATE is enrolled in that transaction and lost
+    // only permit holders that open a BEGIN. On the shared SQLite connection a
+    // bare UPDATE issued while one of those is open is enrolled in it, and lost
     // with it if it rolls back. Losing THIS write loses a drift signal outright.
+    // The claim reaches no further: transactions in this file that do not take
+    // the permit are not serialized against this write.
     const connectionsMarkToolsStale = (ref: ConnectionRef): Effect.Effect<void, StorageFailure> =>
       withCatalogPersistLock(
         core.updateMany("connection", {

packages/core/sdk/src/plugin.ts+6−1

      *  Safe to call at any point, including while a listing for the same
      *  connection is in flight: the mark is recorded as a fresh token, and a
      *  listing only resolves the token it observed when it started. A signal
-     *  that arrives mid-listing therefore survives it and re-lists. */
+     *  that arrives mid-listing therefore survives it and re-lists.
+     *
+     *  NOT from inside `ctx.transaction(...)`, though: the write takes a
+     *  non-reentrant executor permit, and on sqlite an open BEGIN blocking on
+     *  that permit can silently drop a concurrent catalog rebuild. Call it
+     *  before the transaction, or after it commits. */
     readonly markToolsStale: (ref: ConnectionRef) => Effect.Effect<void, StorageFailure>;
     /** Resolve a connection's value through its provider (and OAuth refresh).
      *  null if the provider can't produce one. */

packages/core/sdk/src/tools-sync-scope.test.ts+2−1

                 ? Effect.die(`no connection row named ${name}`)
                 : Effect.succeed({
                     syncedAt: row.tools_synced_at == null ? null : Number(row.tools_synced_at),
-                    staleToken: row.tools_stale_token ?? null,
+                    staleToken:
+                      row.tools_stale_token == null ? null : String(row.tools_stale_token),
                     failures: row.tools_sync_failures == null ? 0 : Number(row.tools_sync_failures),
                     retryAt:
                       row.tools_sync_retry_at == null ? null : Number(row.tools_sync_retry_at),