Repository navigation
spec: optional "ran, but the work did not happen" report on JobHandler (#6617) - #7050
Merged
Merged
Conversation
JobHandler had two states — threw or did not — so a run that completed without accomplishing its work was recorded as success. Widen the return type to Promise<void | JobRunOutcome> so a handler can OPTIONALLY report "ran to completion, work did not happen". Additive on both sides: existing Promise<void> handlers are unchanged byte for byte, and existing IJobService implementations are unchanged because this widens a return type rather than adding a context member. degraded is not a failure and does not retry — retry stays throw-driven. Spec half of #5548's B-minimal ruling; the DbJobAdapter/sys_job_run mapping is #5548's half and is deliberately not wired here. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PiRUoQkTSBBmpyXBY3cVn2
The widened JobHandler may resolve a JobRunOutcome, and a wrapper typed `() => Promise<void>` rejects that — TypeScript's return-type void special case does not reach through Promise<void>, so both the cron and interval adapters failed to build. runWithPolicy/withTimeout are now generic with T = void: every existing caller still infers void and behaviour is unchanged, but the wrapper no longer erases what the run resolved to. Retry stays throw-driven, so a resolved degraded report returns on the first attempt. Also commits the regenerated api-surface shard (+ JobRunOutcome, the only delta: 0 breaking, 1 added). The sys_job_run mapping that CONSUMES the outcome remains #5548's half. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PiRUoQkTSBBmpyXBY3cVn2
) Prime Directive #4 — @objectstack/spec subpaths, matching the sibling adapters; JobHandler/JobRunOutcome are not on the package root. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PiRUoQkTSBBmpyXBY3cVn2
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📓 Docs Drift CheckThis PR changes 2 package(s): 106 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
⛔ 7 release-owned page(s) also reference the affected code. These are read-only:
|
os-zhuang
marked this pull request as ready for review
August 9, 2026 12:13
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6617
The spec half of the maintainer's B-minimal ruling on #5548, split contract-first:
packages/specis the spec seat's lane, and the services half (#5548 — theDbJobAdaptermapping, thesys_job_run.statusenum,bumpJob) is not implemented here.The problem
JobHandlerhad exactly two states — it threw, or it did not — so a run that completed without accomplishing anything was recorded assuccess, 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, andsys_job_runsays the wake succeeded.The change
Three states, of which the third is optional: throw means failed (and retries), resolve nothing means success exactly as today, resolve a degraded outcome means the run finished but its work did not happen.
degradedis not a failure and does not trigger a retry. Failure and retry remain driven exclusively by a rejected promise, so a resolved outcome never re-runs the job and never surfaces as an error. That separation is the point of the ruling — option A (make these handlers throw) was rejected precisely because it would change failure semantics that third-partyIJobServiceimplementations already build retry behaviour on.Why the return-value shape
The API shape was delegated to the spec seat and ruled as a return-value union rather than a
ctx.reportOutcomecallback: the handler context is an inline anonymous object that eachIJobServiceimplementation constructs itself, so a context callback would force every third-party implementation to grow a new member — violating the ruling's additivity clause on the implementer side. A return-value union is additive on both sides.The ruling, verbatim and untranslated:
The issue's falsification clause required grepping for a contrary outcome-reporting precedent on a sibling handler contract, and stopping if one existed. None exists —
report(Outcome|Progress|Result|Status|Degraded)has zero hits acrosspackages/. The two nearest precedents both point the same way as the ruling:packages/spec/src/automation/node-executor.zod.ts:346— the sibling automation handler contract reports its out-of-band result by returning it: "an executor suspends by RETURNINGsuspend: truefromexecute()".packages/spec/src/contracts/metadata-service.ts:292— degraded reporting in the samecontracts/directory, also by return value:Promise< { data; degraded: boolean; errors } >.Naming precedent for the vocabulary is
integration/connector-descriptor.ts:104(degradedReason); the ruling's own{ outcome, reason }field names are kept, since the object is entirely about the outcome and self-describes.Additivity — the acceptance criterion, pinned in both directions
Promise< void >handlers unchangedJobHandlerapi-surfacedelta is exactly+ JobRunOutcome (interface)— 0 breaking, 1 addedpnpm --workspace-concurrency=2 --filter '...@objectstack/spec' typecheck— prefix direction, i.e. spec plus its dependents (74 of 78 projects), all green. Zero-change compilation of every existing consumer is the empirical additivity proof (#6218).sys_job_run's status enum is untouched; extending it is #5548's half.One consumer did need widening — reported rather than papered over
The consumption-radius sweep turned up a genuine exception to the "no consumer changes" story, and it is worth stating plainly:
runWithPolicyin@objectstack/service-jobtyped its run as() => Promise< void >, which rejects a handler that may resolve an outcome. TypeScript's return-typevoidspecial case does not reach throughPromise< void >, so the cron and interval adapters failed to build:Fixed at the wrapper rather than the call sites:
runWithPolicy/withTimeoutare now generic withT = void, so every existing caller still infersvoidand behaviour is unchanged — what changed is that the retry wrapper no longer erases what the run resolved to. Erasing it at the call sites instead would have put a value-discarding shim at exactly the seam #5548 needs to read, which is the consumer-side accommodation Prime Directive #12 rules out. Retry semantics are untouched, and there is a test pinning that a degraded report survives the wrapper and does not retry.Reverse verification
Predicted before running: narrowing the union back to
Promise< void >should turn the degraded pins red while every legacy pin stays green — the asymmetry is what distinguishes additive widening from replacement.The first attempt aimed at the wrong program and is worth recording:
tsc --noEmitreadspackages/spec/tsconfig.json, which excludes**/*.test.ts, so it reported no errors under the narrowed union — the pins live in the test layer, which is checked bycheck:test-typecheckagainst the siblingtsconfig.test.json. Re-aimed at that gate, the prediction held exactly:job-service.test.tsis not intest-typecheck-debt.json, so it must carry zero type errors — which is what makes these pins live rather than phantom.Verification
67 gates enumerated one by one from
.github/workflows/lint.yml(both jobs), pluscheck:adr-0087-registration --base origin/main(exit 0 — an additive minor needs no disposition). All green.check:generatedreports every artifact current; the only regenerated artifact is theapi-surfaceshard above.Generated by Claude Code