fix(objectql): a by-id delete the driver answers as removing nothing leaves no recycle-bin entry

.changeset/31630-recycle-bin-no-ghost-entry.md

@@ -0,0 +1,10 @@
+---
+'@objectstack/objectql': patch
+---
+
+fix(objectql): a by-id delete the driver answers as removing nothing leaves no recycle-bin entry
+
+Clause-②: no
+Metadata-protocol: none
+
+The recycle bin writes its entry before the record's row is removed, so a bin that cannot take the entry refuses the delete before anything goes. When the driver then answers that it removed nothing (`false`, or a count of `0`: the row was already gone, filtered out, or lost a race), the entry used to stay, naming a record that is still live. Admins saw it in the recycle-bin list, and a restore of it was refused `409 RESTORE_UNIQUE_VALUE_TAKEN`, until the retention sweep reaped it. The engine now removes that entry in the same unit of work, inside the delete's transaction when one covers the bin, so the recycle-bin list no longer shows such entries. A delete the driver confirms still writes exactly one entry, its `recycleBinEntryId` answer is unchanged, and the snapshot, restore and purge are untouched. If the entry cannot be removed, the engine logs it once at `error` and the delete's answer stays the same.

packages/objectql/src/engine.ts

@@ -9055,9 +9055,13 @@ export class ObjectQL implements IObjectQLEngine {
               // walk touches it, and writes the one entry after the walk and
               // before this row goes. A removal that throws takes the entry
               // with it: the transaction's rollback when one covers the bin,
-              // the capture's compensation otherwise. A caller's `multi: true`
-              // opens none, even here: the lifecycle sweep reaps by id with it.
-              // Nor does a permanent delete, which is the point of it.
+              // the capture's compensation otherwise. [#31630] A removal the
+              // driver answers as removing nothing withdraws the entry inside
+              // the same unit, so no entry stays for a row that is still live
+              // (`capture.ts` says why it is not written after instead).
+              // A caller's `multi: true` opens none, even here: the lifecycle
+              // sweep reaps by id with it. Nor does a permanent delete, which
+              // is the point of it.
               //
               // [#30237] The entry's id is told to the caller
               // (`options.onRecycleBinEntry`) only once the unit has landed:
@@ -9076,8 +9080,10 @@ export class ObjectQL implements IObjectQLEngine {
                   );
                   // Told only for a row the driver says it removed: `false`
                   // (not found) and an off-contract count of 0 (kept) name no
-                  // deletion an undo could reverse.
+                  // deletion an undo could reverse, and their entry is
+                  // withdrawn.
                   if (removed !== false && (removed as unknown) !== 0) binEntryId = entry?.id;
+                  else await entry?.withdraw();
                   return removed;
                 } catch (e) {
                   await entry?.compensate();

packages/objectql/src/recycle-bin/capture.test.ts

@@ -37,7 +37,7 @@ function recordingLogger() {
   };
 }
 
-function makeDriver(opts: { transactional?: boolean; failDeleteOf?: string } = {}) {
+function makeDriver(opts: { transactional?: boolean; failDeleteOf?: string; removesNothingOf?: { object: string; answer: boolean | number } } = {}) {
   const stores = new Map<string, Map<string, Row>>();
   const storeFor = (o: string) => {
     let s = stores.get(o);
@@ -113,6 +113,9 @@ function makeDriver(opts: { transactional?: boolean; failDeleteOf?: string } = {
     async delete(o: string, id: string, options: any) {
       if (opts.failDeleteOf === o) throw new Error(`driver refused the delete of ${o}/${id}`);
       writes.push({ object: o, op: 'delete', transaction: options?.transaction });
+      // The driver answers that it removed nothing, and keeps the row: a row
+      // already gone, filtered out, or a lost race, as the engine sees it.
+      if (opts.removesNothingOf?.object === o) return opts.removesNothingOf.answer;
       return storeFor(o).delete(String(id));
     },
     async deleteMany(o: string, ast: any, options: any) {
@@ -215,7 +218,7 @@ const ephemeral = {
 /** Nothing references it: the plain, non-cascading delete. */
 const loner = { name: 'loner', label: 'Loner', fields: { id: text('id'), name: text('name') } };
 
-async function makeEngine(options: { withBin?: boolean; transactional?: boolean; failDeleteOf?: string } = {}) {
+async function makeEngine(options: { withBin?: boolean; transactional?: boolean; failDeleteOf?: string; removesNothingOf?: { object: string; answer: boolean | number } } = {}) {
   const rec = recordingLogger();
   const engine = new ObjectQL({ logger: rec.logger } as any);
   const driver = makeDriver(options);
@@ -433,6 +436,65 @@ describe('a failed delete leaves no entry', () => {
   });
 });
 
+describe('a delete the driver answers as removing nothing leaves no entry (#31630)', () => {
+  const binOps = (driver: any) => driver.writes.filter((w: any) => w.object === 'sys_recycle_bin');
+
+  it('a removal the driver confirms keeps exactly one entry', async () => {
+    const { engine, driver } = await makeEngine({ transactional: false });
+    driver.seed('loner', { id: 'l1', name: 'x' });
+    await expect(engine.delete('loner', { where: { id: 'l1' } } as any)).resolves.toBe(true);
+    expect(driver.rowsOf('loner')).toHaveLength(0);
+    expect(entriesOf(driver)).toHaveLength(1);
+    expect(binOps(driver).map((w: any) => w.op)).toEqual(['create']);
+  });
+
+  it.each<[boolean | number, string]>([
+    [false, 'not found'],
+    [0, 'an off-contract count of none'],
+  ])('withdraws the entry when no transaction covered it, the driver answering %j (%s)', async (answer) => {
+    const { engine, driver, rec } = await makeEngine({ transactional: false, removesNothingOf: { object: 'loner', answer } });
+    driver.seed('loner', { id: 'l1', name: 'x' });
+    const told: string[] = [];
+    await expect(engine.delete('loner', { where: { id: 'l1' }, onRecycleBinEntry: (id: string) => told.push(id) } as any))
+      .resolves.toBe(answer);
+    expect(driver.rowsOf('loner')).toHaveLength(1);
+    expect(entriesOf(driver)).toHaveLength(0);
+    // Written, then withdrawn: not merely never written.
+    expect(binOps(driver).map((w: any) => w.op)).toEqual(['create', 'delete']);
+    expect(told).toEqual([]);
+    expect(rec.at('error')).toEqual([]);
+  });
+
+  it.each<[boolean | number]>([[false], [0]])(
+    'withdraws the entry inside the transaction that wraps the cascade, the driver answering %j',
+    async (answer) => {
+      const { engine, driver } = await makeEngine({ removesNothingOf: { object: 'account', answer } });
+      seedAccountTree(driver);
+      await expect(engine.delete('account', { where: { id: 'acc1' } } as any)).resolves.toBe(answer);
+      expect(driver.rowsOf('account')).toHaveLength(1);
+      expect(entriesOf(driver)).toHaveLength(0);
+      const ops = binOps(driver);
+      expect(ops.map((w: any) => w.op)).toEqual(['create', 'delete']);
+      // The withdrawal rides the unit the entry was written in.
+      expect(ops[1].transaction).toBeDefined();
+      expect(ops[1].transaction).toBe(ops[0].transaction);
+    },
+  );
+
+  it('logs at error, and keeps the answer, when the entry cannot be withdrawn', async () => {
+    const { engine, driver, rec } = await makeEngine({
+      transactional: false,
+      removesNothingOf: { object: 'loner', answer: false },
+      failDeleteOf: 'sys_recycle_bin',
+    });
+    driver.seed('loner', { id: 'l1', name: 'x' });
+    await expect(engine.delete('loner', { where: { id: 'l1' } } as any)).resolves.toBe(false);
+    expect(entriesOf(driver)).toHaveLength(1);
+    // Said once, at `error`: the bin keeps an entry for a row that is still live.
+    expect(rec.at('error')).toHaveLength(1);
+  });
+});
+
 describe('the TTL sweep reaps an entry once its stamped deadline has passed (D3)', () => {
   it('reaps due entries and keeps the rest, asking no resolver', async () => {
     const { engine, driver } = await makeEngine();

packages/objectql/src/recycle-bin/capture.ts

@@ -25,6 +25,19 @@
  *   4. {@link RecycleBinWrittenEntry.compensate}, when the root row's removal
  *      throws and no transaction covered the entry: the entry is removed, so a
  *      failed delete leaves no entry for a row that is still live.
+ *   5. {@link RecycleBinWrittenEntry.withdraw}, when the driver answers that it
+ *      removed nothing (`false`, or an off-contract count of `0`: the row was
+ *      already gone, filtered out, or lost a race): the entry is removed with
+ *      the driver options of this unit of work, inside its transaction when one
+ *      covers the bin. Nothing throws, so no rollback would take the entry, and
+ *      an entry for a deletion that did not happen is a ghost an admin sees.
+ *
+ * The entry is written BEFORE the root row is removed, and withdrawn after a
+ * removal that removed nothing, rather than written only once the removal is
+ * confirmed: with no transaction, a write after the removal that failed would
+ * leave a removed row with no entry to undo it, and nothing could compensate
+ * that. Written first, a bin that cannot take the entry refuses the delete
+ * before the row goes.
  *
  * ## Which deletes enter the bin (v1, seat ruling on #30237 Q6)
  *
@@ -156,6 +169,13 @@ export interface RecycleBinWrittenEntry {
    * replaces the delete's own error.
    */
   compensate(): Promise<void>;
+  /**
+   * Remove the entry when the root row's removal answered that it removed
+   * nothing. Always removes, inside the covering transaction when there is
+   * one: no rollback follows a removal that did not throw. A failure to remove
+   * it is logged at `error` and never changes the delete's answer.
+   */
+  withdraw(): Promise<void>;
 }
 
 /**
@@ -252,27 +272,34 @@ export class RecycleBinCapture {
     const host = this.host;
     const object = this.object;
     const recordId = this.id;
+    /** Remove the entry; `outcome` says, in the logged text, why it goes. */
+    const remove = async (outcome: 'failed' | 'removed nothing'): Promise<void> => {
+      if (id === undefined) {
+        host.logger.error(`[recycle-bin] the delete ${outcome} and its entry cannot be removed: the driver returned no entry id`, undefined, {
+          object, id: recordId,
+        });
+        return;
+      }
+      try {
+        await driver.delete(RECYCLE_BIN_OBJECT, id, options);
+      } catch (error) {
+        // The driver's fault, bound values stripped: it may quote the row.
+        const redacted = redactBoundStatement(error);
+        host.logger.error(
+          `[recycle-bin] the delete ${outcome} and removing its entry failed too; the entry names a record that is still live`,
+          redacted instanceof Error ? redacted : undefined,
+          { object, id: recordId, entry: id },
+        );
+      }
+    };
     return {
       id,
       async compensate(): Promise<void> {
         if (covered) return;
-        if (id === undefined) {
-          host.logger.error('[recycle-bin] the delete failed and its entry cannot be removed: the driver returned no entry id', undefined, {
-            object, id: recordId,
-          });
-          return;
-        }
-        try {
-          await driver.delete(RECYCLE_BIN_OBJECT, id, options);
-        } catch (error) {
-          // The driver's fault, bound values stripped: it may quote the row.
-          const redacted = redactBoundStatement(error);
-          host.logger.error(
-            '[recycle-bin] the delete failed and removing its entry failed too; the entry names a record that is still live',
-            redacted instanceof Error ? redacted : undefined,
-            { object, id: recordId, entry: id },
-          );
-        }
+        await remove('failed');
+      },
+      async withdraw(): Promise<void> {
+        await remove('removed nothing');
       },
     };
   }