Merge pull request 'fix(events): core.lease keeps a module's retry: false on read/apply refusals' (#236) from fix/lease-retry-206 into main

Reviewed-on: #236
Reviewed-by: Colby Whitlock <whitlocktech@gmail.com>

server/src/config/coreEventActions.js

@@ -442,8 +442,12 @@ const ACTIONS = [
 
       const baseline = await lease.read({ target })
       if (!baseline || baseline.ok !== true) {
+        // A module that says `retry: false` has said a retry cannot help, so that
+        // verdict is passed through; anything else stays undefined so the
+        // dispatcher's own default (retry) still applies.
         return {
           ok: false,
+          retry: baseline && baseline.retry === false ? false : undefined,
           error: baseline && baseline.error
             ? String(baseline.error)
             : `could not read the current value of ${lease.label}`,
@@ -494,7 +498,11 @@ const ACTIONS = [
       }
       if (!applied || applied.ok !== true) {
         await resourcesDb.markReverted(reserved.id)
-        return { ok: false, error: applied && applied.error ? String(applied.error) : `${lease.label} refused the new value` }
+        return {
+          ok: false,
+          retry: applied && applied.retry === false ? false : undefined,
+          error: applied && applied.error ? String(applied.error) : `${lease.label} refused the new value`,
+        }
       }
 
       await resourcesDb.confirm(reserved.id)

server/test/eventModuleContract.test.js

@@ -813,6 +813,92 @@ test('a dry run of core.lease checks everything and takes nothing', async () =>
   assert.equal(bad.outcome, 'terminal')
 })
 
+// ── A lease's refusal keeps its `retry: false` (website#206) ────────────────
+//
+// `core.lease` rebuilt both failure returns as `{ ok: false, error }`, so a module
+// that said "a retry cannot help" (a shard that refuses the value outright, a
+// target that does not exist) was dispatched as `retry: true` and the run paused
+// and re-asked on a loop. The module's `retry: false` now passes through, and
+// anything else is left undefined so the dispatcher's own default still applies.
+//
+// The reservation lands in MariaDB, which these tests do not have, so the two
+// calls `core.lease` makes on it are swapped for recorders and put back after.
+
+const REFUSING_LEASES = `module.exports = (ctx, api) => {
+  const lease = (id, read, apply) => ({
+    id, label: 'Gain rate', type: 'float', min: 0.5, max: 5, maxDurationMs: 3600000,
+    read, apply, async restore() { return { ok: true } },
+  })
+  const fine = async () => ({ ok: true, value: 1 })
+  const accepts = async () => ({ ok: true })
+  api.registerEventLeases([
+    lease('demo.read.final', async () => ({ ok: false, retry: false, error: 'no such spawner' }), accepts),
+    lease('demo.read.soft', async () => ({ ok: false, error: 'shard is restarting' }), accepts),
+    lease('demo.apply.final', fine, async () => ({ ok: false, retry: false, error: 'value refused' })),
+    lease('demo.apply.soft', fine, async () => ({ ok: false, error: 'shard is restarting' })),
+  ])
+}`
+
+/** Run `fn` with the lease reservation calls recorded rather than sent to a database. */
+async function withStubbedReservations(fn) {
+  // eslint-disable-next-line global-require
+  const resourcesDb = require('../src/model/events/eventRunResources.db')
+  const original = { reserve: resourcesDb.reserve, markReverted: resourcesDb.markReverted }
+  const calls = { reserved: 0, reverted: [] }
+  resourcesDb.reserve = async () => { calls.reserved += 1; return { ok: true, id: 77 } }
+  resourcesDb.markReverted = async (id) => { calls.reverted.push(id) }
+  try {
+    return await fn(calls)
+  } finally {
+    Object.assign(resourcesDb, original)
+  }
+}
+
+const leaseStep = (lease) => step('core.lease', { lease, value: '3', minutes: 10 })
+
+test('a lease read refusal that says retry: false is terminal, not retried', async () => {
+  assertRegistered(loadModule('demo', REFUSING_LEASES))
+  registries.registerCore()
+
+  await withStubbedReservations(async (calls) => {
+    const result = await dispatch.dispatchStep(leaseStep('demo.read.final'), { run: RUN })
+    assert.equal(result.outcome, 'terminal')
+    assert.match(result.error, /no such spawner/)
+    // Nothing was reserved, so there is nothing to give back.
+    assert.equal(calls.reserved, 0)
+  })
+})
+
+test('a lease apply refusal that says retry: false is terminal, and the reservation is still released', async () => {
+  assertRegistered(loadModule('demo', REFUSING_LEASES))
+  registries.registerCore()
+
+  await withStubbedReservations(async (calls) => {
+    const result = await dispatch.dispatchStep(leaseStep('demo.apply.final'), { run: RUN })
+    assert.equal(result.outcome, 'terminal')
+    assert.match(result.error, /value refused/)
+    assert.equal(calls.reserved, 1)
+    assert.deepEqual(calls.reverted, [77])
+  })
+})
+
+test('a lease refusal that does not say retry: false is still retried', async () => {
+  assertRegistered(loadModule('demo', REFUSING_LEASES))
+  registries.registerCore()
+
+  await withStubbedReservations(async (calls) => {
+    const read = await dispatch.dispatchStep(leaseStep('demo.read.soft'), { run: RUN })
+    assert.equal(read.outcome, 'retry')
+    assert.match(read.error, /shard is restarting/)
+
+    const apply = await dispatch.dispatchStep(leaseStep('demo.apply.soft'), { run: RUN })
+    assert.equal(apply.outcome, 'retry')
+    assert.match(apply.error, /shard is restarting/)
+    // The apply refusal released its reservation exactly as before.
+    assert.deepEqual(calls.reverted, [77])
+  })
+})
+
 // ── Targeted leases and searchable sources (Phase 12b) ──────────────────────
 
 test('a targeted lease is a family of values, and the target rides in the ref', async () => {