docs(core,service-cluster): retire the two docblocks left stale by IPubSub's corrected delivery guarantee (#12836) - #12954
Conversation
…ubSub's corrected delivery guarantee (#12836) #12651 corrected IPubSub's contract docblock to state delivery driver-relatively: no shipped driver exceeds at-most-once, a missed message is EXPECTED, handlers must be idempotent AND tolerate loss. Two docblocks elsewhere still described the pre-#12651 world. - packages/core/src/security/authz-invalidation-channel.ts: the 'known contradiction' paragraph asserted in the present tense that the interface docblock 'still says' At-least-once, and that repairing it was filed separately. That filing landed, so the paragraph is now false, not merely stale. Replaced with a plain pointer to the interface docblock; the rest of the docblock is unchanged. - packages/services/service-cluster/src/memory/pubsub.ts: 'At-least-once semantics held vacuously' was wrong on its own terms even before #12651 -- handler errors are swallowed via onError, so a throwing handler loses the message with no retry and no persistence. Replaced with the honest statement. Prose only. No behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194kbQJxUvv2yvsGRtuXpP5
📓 Docs Drift Check
What this run could not see
Coarse fallback — 26 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): |
Review — ACCEPT, pending CI. Verified against the diff and the tree, not the report's self-description.PM seat Scope and blast radius — held exactly
The dispatch's biggest risk was a full-text search "helpfully" fixing the other nine. It didn't happen. ⭐ The new prose is accurate against the CODE, not just against the cardThe card asserted the old memory-driver line was wrong on its own terms. The dev didn't stop at that claim — it read the implementation and wrote the docblock from it. I re-verified all three legs independently on the branch:
⇒ "no persistence, no retry, no replay" is derived from the code, not paraphrased from the issue. That is the correct standard for a docblock repair, and it made the replacement text strictly better than what the card asked for. Site 1's replacement also does the right thing structurally: it does not merely delete the stale paragraph, it leaves a pointer saying the interface, ⭐ The ablation call — declining to fabricate one, and substituting something betterThis is the part worth recording. Told an ablation was expected in the general case, the dev correctly reported that a comment-only change admits none, and ⛔ did not invent one. In its place it ran a real positive measurement of the very property under review: both changed files compiled at the merge base and at head with That instrument is better designed than an ablation would have been here, and it does two jobs at once:
It also disposes of the one loose end honestly: Reporting discipline
My false-green warning was confirmed rather than assumed: neither package declares a
|
Fixes #12836
#12651 (PR #12837) corrected
IPubSub's contract docblock: delivery is whatever the configured driver declares, no shipped driver exceeds at-most-once, a missed message is EXPECTED, handlers must be idempotent and tolerate loss, anddeliverySemanticsis the per-channel ask-not-get surface.Two docblocks elsewhere still described the pre-#12651 world. This PR retires both. Prose only — no behaviour change, no test change.
Site 1 —
packages/core/src/security/authz-invalidation-channel.tsThe docblock carried a paragraph asserting, in the present tense, that the interface docblock "still says" At-least-once delivery, and that repairing it "is a
packages/specchange and is filed separately, deliberately not made here."That paragraph declared its own expiry condition, and the condition is now met: the separate filing was #12651, and it has landed. Leaving it is worse than never having written it — the next author goes looking for a live disagreement between the interface and the drivers, and finds none. Replaced with a plain pointer to the now-honest interface docblock.
Everything else in that docblock is unchanged, verbatim, because it is all still correct: the at-most-once reasoning with its three measured citations, the TTL-is-the-bound rule, and the best-effort-at-the-publish-site note ("a publish failure is logged and swallowed, never propagated into the write that triggered it").
Site 2 —
packages/services/service-cluster/src/memory/pubsub.tsThe line was:
⭐ This line was wrong on its own terms even before #12651, which is what makes the change more than a consequential edit. The same docblock, two bullets up, states that handler errors are "swallowed and logged via
onError(so one bad subscriber can't poison the bus)". A handler that throws therefore loses the message outright: no retry, no persistence, no replay. That is not at-least-once in any sense — "vacuously" does not save it, because the single delivery attempt can itself fail and nothing redelivers.Confirmed against the implementation in the same file, not only its prose:
throwgoes tothis.onError(err, channel)and is dropped;.catch((err) => this.onError(err, channel))and is dropped;publish()returns early when a channel has no subscriber bucket, so a publish nobody is subscribed to at that moment is a silent no-op.Replaced with the honest statement: at-most-once, one synchronous in-process attempt per subscriber, no persistence, no retry, no replay.
Blast radius — the nine other
At-least-oncesites are deliberately untouchedA repo-wide grep for
At-least-oncereturned eleven hits before this change and nine after. The nine that remain are all different subjects, and two of them sit on governed surfaces:packages/triggers/trigger-api/src/api-trigger.tsdocs/adr/0041-flow-trigger-family.mddocs/adr/0119-plugin-reachable-transactions-and-honest-atomic-batch.md.changeset/ipubsub-delivery-guarantee-docblock.md(2 hits)content/docs/automation/webhooks.mdxpackages/objectql/src/engine.tspackages/services/service-messaging/src/dispatcher.tspackages/services/service-messaging/src/http-dispatcher.tsVerification
Every run below is on
e50b1d2dd, this PR's head commit, with a clean working tree.Prose-only, proven rather than asserted. Both changed source files were compiled at the merge base and at head with
removeComments: true, and the emitted output compared byte for byte:There is no ablation leg in this PR, deliberately. This is a comment-only change: no pin can be made to go red by mutating prose, so an ablation here would be fabricated rather than measured.
Tests — both affected packages, green:
pnpm --filter @objectstack/core test—Test Files 41 passed (41),Tests 1015 passed (1015)pnpm --filter @objectstack/service-cluster test—Test Files 4 passed (4),Tests 66 passed (66)Lint — repo-wide, not narrowed:
pnpm lint(eslint . --no-inline-config) exits 0 with no findings, so no narrowing is claimed or owed.Gate families — derived from the actual changed set by
node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack, which read the merge base itself (3 paths, commite50b1d2dd, repo assertion held). All 21 named families are green, pluscheck:nul-bytes. Selected judgment lines, each as printed by the gate itself:Typecheck. Neither
@objectstack/corenor@objectstack/service-clusterdeclares atypecheckscript, so a filteredrun typecheckwould match nothing and exit 0 — not a measurement. Rannpx tsc --noEmit -p tsconfig.jsonfrom inside each package instead. Both exit 2, entirely on pre-existing diagnostics in test files these packages hide from tsc; both sit in the type-check DEBT ledger, andcheck-type-check-coverageis green on that ledger. The comment-stripped-emit proof above is what establishes this PR cannot have moved those counts.One gate NOT MEASURED — reported as such, not folded into the green list.
node scripts/pm/check-half-states.mjs(the live board sweep, whose standing caller ishalf-state-patrol.yml) exits 3:That is an environment classification, not a verdict on this PR, and
lint.ymlsays so where it wires the gate: the live sweep's "non-zero exits classify the ENVIRONMENT — no token, exhausted quota, unreachable host — which is not a verdict about the PR running it." The form CI runs in the PR lane is the offline self-test,pnpm check:pm-half-states, which is green here.Generated by Claude Code