spec: optional "ran, but the work did not happen" report on JobHandler (#6617)

.changeset/job-handler-degraded-outcome.md

@@ -0,0 +1,58 @@
+---
+"@objectstack/spec": minor
+"@objectstack/service-job": minor
+---
+
+feat(spec): give `JobHandler` an optional "ran, but the work did not happen" report (#6617)
+
+`JobHandler` had exactly two states — it threw, or it did not — so a run that
+completed without accomplishing anything was recorded as `success`,
+indistinguishable from one that did the work. The motivating case is #5529's
+wait-wake handler: when its store is unavailable it fires the shot at nothing,
+returns normally, and `sys_job_run` says the wake succeeded.
+
+This is the **spec half** of the maintainer's B-minimal ruling on #5548. The
+handler's return type widens:
+
+```ts
+export interface JobRunOutcome {
+  outcome: 'completed' | 'degraded';
+  reason?: string;
+}
+
+export type JobHandler =
+  (context: { jobId: string; data?: unknown }) => Promise<void | JobRunOutcome>;
+```
+
+**`degraded` is not a failure and does not trigger a retry.** Failure and retry
+remain driven exclusively by a rejected promise (`runWithPolicy` retries on
+throw), so a resolved outcome — whatever it says — never re-runs the job and
+never surfaces as an error. A handler that wants a retry must still throw. That
+separation is the point of the ruling: making these handlers throw instead was
+rejected precisely because it would change failure semantics that third-party
+`IJobService` implementations already build retry behaviour on.
+
+**Nothing to migrate — this is additive on both sides.**
+
+- Existing `Promise< void >` handlers are unchanged, byte for byte. Reporting
+  nothing means exactly what it means today: no throw implies success.
+- Existing `IJobService` implementations are unchanged. This widens a *return*
+  type rather than adding a member to the handler context, so no implementation
+  has to grow anything; an adapter that ignores the resolved value keeps its
+  current behaviour. (A `ctx.reportOutcome` callback would have forced every
+  third-party implementation to construct a new context member — which is why
+  the return-value shape was chosen.)
+
+**One consumer needed widening too, and it is additive as well.**
+`runWithPolicy` in `@objectstack/service-job` typed its run as
+`() => Promise< void >`, which rejects a handler that may resolve an outcome —
+TypeScript's return-type `void` special case does not reach through
+`Promise< void >`. It is now generic with `T = void`, so every existing call
+still infers `T = void` and behaviour is unchanged; what changed is that the
+retry wrapper no longer *erases* what the run resolved to. Retry semantics are
+untouched: only a rejected promise retries.
+
+Consuming the report — mapping `degraded` onto a `sys_job_run.status` distinct
+from `success` — is the services half, tracked in #5548, and is deliberately not
+wired here: the shipped adapters currently discard the resolved value, which is
+what makes landing the contract first safe.

packages/services/service-job/src/run-with-policy.outcome.test.ts

@@ -0,0 +1,66 @@
+import { describe, it, expect } from 'vitest';
+import type { JobHandler, JobRunOutcome } from '@objectstack/spec/contracts';
+import { runWithPolicy } from './run-with-policy.js';
+
+/**
+ * [#6617] `runWithPolicy` must not ERASE what a job run resolved to.
+ *
+ * `JobHandler` may now resolve a {@link JobRunOutcome}. A wrapper typed
+ * `() => Promise<void>` rejects that at compile time — TypeScript's
+ * return-type `void` special case does not reach through `Promise<void>` —
+ * so the wrapper is generic with `T = void`. These cases pin the two halves:
+ * the value survives the wrapper, and retry stays throw-driven so a degraded
+ * report never re-runs the job.
+ *
+ * The `sys_job_run` mapping that CONSUMES the outcome is #5548's half and is
+ * deliberately not implemented here.
+ */
+describe('[#6617] runWithPolicy passes the run outcome through', () => {
+  it('returns a degraded outcome unchanged, with no retry policy', async () => {
+    const handler: JobHandler = async () => ({ outcome: 'degraded', reason: 'STORE_UNAVAILABLE' });
+
+    const result = await runWithPolicy('wait_wake', () => handler({ jobId: 'wait_wake' }));
+
+    expect(result).toEqual({ outcome: 'degraded', reason: 'STORE_UNAVAILABLE' });
+  });
+
+  it('returns a degraded outcome unchanged THROUGH the retry path, without retrying', async () => {
+    let attempts = 0;
+    const handler: JobHandler = async () => {
+      attempts++;
+      return { outcome: 'degraded', reason: 'nothing to wake' } satisfies JobRunOutcome;
+    };
+
+    const result = await runWithPolicy(
+      'wait_wake',
+      () => handler({ jobId: 'wait_wake' }),
+      { retryPolicy: { maxRetries: 3, backoffMs: 1 } },
+    );
+
+    // Resolved is resolved: degraded is not a failure, so one attempt only.
+    expect(attempts).toBe(1);
+    expect(result).toEqual({ outcome: 'degraded', reason: 'nothing to wake' });
+  });
+
+  it('a legacy void handler still resolves undefined — unchanged', async () => {
+    const legacy: JobHandler = async () => {};
+
+    const result = await runWithPolicy('sync_metadata', () => legacy({ jobId: 'sync_metadata' }));
+
+    expect(result).toBeUndefined();
+  });
+
+  it('a throwing handler still retries and rethrows — unchanged', async () => {
+    let attempts = 0;
+
+    await expect(
+      runWithPolicy(
+        'flaky',
+        async () => { attempts++; throw new Error('boom'); },
+        { retryPolicy: { maxRetries: 2, backoffMs: 1 } },
+      ),
+    ).rejects.toThrow('boom');
+
+    expect(attempts).toBe(3); // initial + 2 retries
+  });
+});

packages/services/service-job/src/run-with-policy.ts

@@ -40,14 +40,14 @@ function sleep(ms: number): Promise<void> {
   });
 }
 
-function withTimeout(run: () => Promise<void>, jobId: string, timeoutMs?: number): Promise<void> {
+function withTimeout<T>(run: () => Promise<T>, jobId: string, timeoutMs?: number): Promise<T> {
   if (!timeoutMs || timeoutMs <= 0) return run();
   let timer: ReturnType<typeof setTimeout> | undefined;
   const guard = new Promise<never>((_, reject) => {
     timer = setTimeout(() => reject(new JobTimeoutError(jobId, timeoutMs)), timeoutMs);
     (timer as any)?.unref?.();
   });
-  return Promise.race([run(), guard]).finally(() => clearTimeout(timer)) as Promise<void>;
+  return Promise.race([run(), guard]).finally(() => clearTimeout(timer)) as Promise<T>;
 }
 
 /**
@@ -67,12 +67,22 @@ function withTimeout(run: () => Promise<void>, jobId: string, timeoutMs?: number
  *   convergence (#4661) and are enforced here, not merely declared: jitter is
  *   what stops a fleet of jobs that failed on the same outage from retrying in
  *   lockstep.
+ *
+ * Generic in the run's resolved value, defaulting to `void` (#6617). Every
+ * existing caller passes a `() => Promise<void>` and still infers `T = void`,
+ * so this is purely additive — what changed is that the wrapper no longer
+ * ERASES what the run resolved to. It has to stop erasing it because
+ * {@link JobHandler} can now resolve a `JobRunOutcome`, and a wrapper typed
+ * `() => Promise<void>` rejects that: TypeScript's return-type `void` special
+ * case does not reach through `Promise<void>`. Retry semantics are untouched —
+ * only a REJECTED promise retries, so a resolved outcome (degraded or not)
+ * returns on the first attempt exactly as a resolved `undefined` always did.
  */
-export async function runWithPolicy(
+export async function runWithPolicy<T = void>(
   jobId: string,
-  run: () => Promise<void>,
+  run: () => Promise<T>,
   options?: JobScheduleOptions,
-): Promise<void> {
+): Promise<T> {
   const timeoutMs = options?.timeout;
   if (!options?.retryPolicy) {
     return withTimeout(run, jobId, timeoutMs);
@@ -93,8 +103,7 @@ export async function runWithPolicy(
       await sleep(delay);
     }
     try {
-      await withTimeout(run, jobId, timeoutMs);
-      return;
+      return await withTimeout(run, jobId, timeoutMs);
     } catch (err) {
       lastError = err;
     }

packages/spec/api-surface/contracts.json

@@ -163,6 +163,7 @@
     "JobExecution (type)",
     "JobHandler (type)",
     "JobRetryPolicy (interface)",
+    "JobRunOutcome (interface)",
     "JobSchedule (interface)",
     "JobScheduleOptions (interface)",
     "KNOWLEDGE_SERVICE (const)",

packages/spec/src/contracts/job-service.test.ts

@@ -1,5 +1,5 @@
 import { describe, it, expect } from 'vitest';
-import type { IJobService, JobHandler, JobExecution } from './job-service';
+import type { IJobService, JobHandler, JobRunOutcome, JobExecution } from './job-service';
 
 describe('Job Service Contract', () => {
   it('should allow a minimal IJobService implementation with required methods', () => {
@@ -109,3 +109,138 @@ describe('Job Service Contract', () => {
     expect(jobs).toContain('cleanup_logs');
   });
 });
+
+/**
+ * [#6617] The degraded-outcome reporting channel — the spec half of #5548's
+ * B-minimal ruling.
+ *
+ * These are COMPILE-TIME pins first and runtime assertions second: the package
+ * type-checks its tests through `tsconfig.test.json`, so an assignability pin
+ * here is a real check rather than a phantom one. Reverse verification for the
+ * whole block is `JobHandler`'s return type narrowed back to `Promise<void>` —
+ * which reddens the `degraded` direction only, and leaves every legacy pin
+ * green. That asymmetry IS additivity.
+ */
+describe('[#6617] JobHandler degraded-outcome channel', () => {
+  /**
+   * The handler type EXACTLY as it stood before #6617. Pinning it as a
+   * standalone declaration is what makes the additivity claim falsifiable:
+   * if the union ever stops being a widening, these assignments break.
+   */
+  type PreIssue6617JobHandler = (context: { jobId: string; data?: unknown }) => Promise<void>;
+
+  it('(a) an existing Promise< void > handler is unchanged — additivity, implementer side', async () => {
+    // Written the way every handler in the repo is written today: no return.
+    const legacy: PreIssue6617JobHandler = async (ctx) => {
+      void ctx.jobId;
+    };
+
+    // The pin: the PRE-change type is still assignable to the post-change one.
+    // A narrowing (rather than a widening) of JobHandler breaks this line.
+    const stillAJobHandler: JobHandler = legacy;
+
+    // …and inline literals keep inferring, with no annotation ceremony.
+    const inlineLegacy: JobHandler = async () => {};
+
+    await expect(stillAJobHandler({ jobId: 'sync_metadata' })).resolves.toBeUndefined();
+    await expect(inlineLegacy({ jobId: 'sync_metadata' })).resolves.toBeUndefined();
+  });
+
+  it('(b) a handler reporting { outcome: degraded, reason } is type-legal', async () => {
+    // THE pin that must go red when the union is narrowed back to Promise< void >.
+    const degraded: JobHandler = async () => ({
+      outcome: 'degraded',
+      reason: 'STORE_UNAVAILABLE',
+    });
+
+    const completed: JobHandler = async () => ({ outcome: 'completed' });
+
+    await expect(degraded({ jobId: 'wait_wake' })).resolves.toEqual({
+      outcome: 'degraded',
+      reason: 'STORE_UNAVAILABLE',
+    });
+    await expect(completed({ jobId: 'wait_wake' })).resolves.toEqual({ outcome: 'completed' });
+  });
+
+  it('reason is optional, and the outcome union admits exactly two members', async () => {
+    const bare: JobRunOutcome = { outcome: 'degraded' };
+    expect(bare.reason).toBeUndefined();
+
+    // @ts-expect-error 'skipped' is not a member of the outcome union — the
+    // third state is `degraded`, and new states are a spec decision, not a
+    // free-text field. (This directive is live: the tests are type-checked.)
+    const bogus: JobRunOutcome = { outcome: 'skipped' };
+    expect(bogus.outcome).toBe('skipped');
+  });
+
+  it('an adapter that IGNORES the resolved value keeps todays behaviour exactly', async () => {
+    // This is `DbJobAdapter.wrap()`'s shape as it stands on main (L161-176):
+    // `await handler(ctx)` discards the resolved value, and only a THROW takes
+    // the failure branch. #5548 is what teaches it to read the value.
+    const recorded: string[] = [];
+    const wrap = (handler: JobHandler): JobHandler => async (ctx) => {
+      try {
+        await handler(ctx);
+        recorded.push('success');
+      } catch {
+        recorded.push('failed');
+      }
+    };
+
+    await wrap(async () => {})({ jobId: 'legacy' });
+    await wrap(async () => ({ outcome: 'degraded', reason: 'nothing to wake' }))({ jobId: 'new' });
+
+    // Both land on `success` — an unwired adapter cannot regress, which is the
+    // safety argument for landing the spec half first.
+    expect(recorded).toEqual(['success', 'success']);
+  });
+
+  it('a consumer that DOES read the value distinguishes all three states', async () => {
+    // The mapping #5548 will implement, pinned here at the contract level so
+    // the spec half states what the services half owes.
+    const classify = async (handler: JobHandler): Promise<'success' | 'degraded' | 'failed'> => {
+      let result: void | JobRunOutcome;
+      try {
+        result = await handler({ jobId: 'j' });
+      } catch {
+        return 'failed';
+      }
+      // Reporting nothing is success — "不回报 = 现状".
+      return result && result.outcome === 'degraded' ? 'degraded' : 'success';
+    };
+
+    expect(await classify(async () => {})).toBe('success');
+    expect(await classify(async () => ({ outcome: 'completed' }))).toBe('success');
+    expect(await classify(async () => ({ outcome: 'degraded', reason: 'x' }))).toBe('degraded');
+    expect(await classify(async () => { throw new Error('boom'); })).toBe('failed');
+  });
+
+  it('degraded does not travel the retry path — retry is throw-driven', async () => {
+    // Mirrors `runWithPolicy`: retries are driven by a REJECTED promise, so a
+    // resolved degraded report can never re-run the job.
+    let attempts = 0;
+    const runWithRetry = async (handler: JobHandler, maxRetries: number) => {
+      for (let i = 0; i <= maxRetries; i++) {
+        try {
+          return await handler({ jobId: 'j' });
+        } catch (err) {
+          if (i === maxRetries) throw err;
+        }
+      }
+    };
+
+    const degraded: JobHandler = async () => {
+      attempts++;
+      return { outcome: 'degraded', reason: 'STORE_UNAVAILABLE' };
+    };
+
+    const outcome = await runWithRetry(degraded, 3);
+    expect(attempts).toBe(1); // ← not 4: degraded is not a failure
+    expect(outcome).toEqual({ outcome: 'degraded', reason: 'STORE_UNAVAILABLE' });
+
+    let thrownAttempts = 0;
+    await expect(runWithRetry(async () => { thrownAttempts++; throw new Error('boom'); }, 3))
+      .rejects.toThrow('boom');
+    expect(thrownAttempts).toBe(4); // ← a throw still retries, unchanged
+  });
+});