Skip to content

fix(plugin-webhooks): report a webhook delivery the outbox could not record at error, once per webhook - #24862

Merged
objectstack-fleet[bot] merged 2 commits into
mainfrom
claude/issue-24835-pg-webhook-enqueue-circular
Oct 20, 2026
Merged

objectstack-fleet[bot] merged 2 commits into
mainfrom
claude/issue-24835-pg-webhook-enqueue-circular

Conversation

@objectstack-fleet

Copy link
Copy Markdown
Contributor

Part of #24835

Clause-②: no

#24835 remains open after this PR: it carries the log-level half only. The delivery loss itself has a measured root cause in packages/drivers/driver-sql (with the event built in packages/objectql), which is domain:engine's code, so it is reported on the card as a fork and not patched here.

What this PR changes

When the sys_http_delivery write for a webhook event failed, AutoEnqueuer.reportWriteFailure logged one bare WARN [webhook-auto-enqueuer] enqueue failed per event. Under AGENTS.md "Degradation log levels" that is a durability loss, not a functional one. The record write that produced the event already answered 200, the webhook still reads active, and no delivery row exists, so the event is never sent, retried or redeliverable, and nothing else looks wrong.

  • The first lost delivery for a webhook is now logged at error. The line names the webhook, says the event is lost and why nothing else shows it, and says where the cause is (the record's meta) and what to restore.
  • It is said once per webhook (writeFailing, the enqueue twin of droppedForSecret). Each further lost event for that webhook is kept at debug with its eventId.
  • The next delivery recorded for that webhook clears the mark, so a later outage is loud again. Both enqueue call sites now route the success outcome too (.then(recordedWrite, reportWriteFailure)).
  • A logger without error still gets the line at warn (the file's existing fallback). The parked-subscription branch is unchanged.
  • No edit falls inside the mirror and declared-name region (the region the merged step-7c change rewrote). All hunks start at line 680 or later.

Measured root cause of the PostgreSQL loss (not changed here)

I reproduced it with the real ObjectQL engine and the real SqlDriver on a throwaway local PostgreSQL 16.15 cluster, and in-memory SQLite. The real AutoEnqueuer was wired the production way to the real MessagingService and SqlHttpOutbox. I ran one insert and one by-id update, then read sys_http_delivery:

run rows non-plain value in the data.record.updated payload log
PG, base 41646a5788 1 (created only) payload.changes.updated_at is a knex Raw WARN … enqueue failed … Converting circular structure to JSON … 'Timeout'
SQLite, base 2 none none
PG, this branch at 9a8a7f5103, 2 updates 1 same Raw one ERROR naming the webhook, then one DEBUG for the second loss
PG at 9a8a7f5103, driver handed a copy of the patch, 2 updates 3 none none

The chain, at 41646a5788:

  1. ObjectQL.update passes hookContext.input.data to driver.update (packages/objectql/src/engine.ts:18094). It then publishes the same object as DataEvent.changes (:18530).
  2. SqlDriver.update builds formatted from formatInput (packages/drivers/driver-sql/src/sql-driver.ts:8767). For a patch that needs no coercion, formatInput returns the caller's object itself (:20336, :20526), and so does applyWriteColumnMap for a managed object. Then formatted.updated_at = this.updatedAtStamp() (:8778) writes into the caller's patch.
  3. On PostgreSQL and MySQL, updatedAtStamp() is knex.fn.now() (:8579), a Raw that holds the knex client, its pool and a Node Timeout. On SQLite it is an ISO string (:8578). That string is just as foreign to the caller's patch, but it serialises.
  4. projectRecordBodies copies changes shallowly, and sql-http-outbox.ts:192 JSON.stringify throws on it.

rotatedUpdateById has the same in-place stamp (:10207). updateMany already builds a fresh object instead (:10152). The fork, the candidate fix sites and the evidence are on the card.

Not added: a serialisation guard in sql-http-outbox.ts

Reading of the dispatch's decision frame: it does not belong beside the root-cause fix.

  • Real business need. The only measured producer of a non-plain value is the driver stamp. Once that is fixed upstream, a guard has no measured pull.
  • Long-term soundness. Fixing the producer is the contract-first route. A guard is a second place that knows about payload shape.
  • Preventing AI mistakes. The outbox already refuses: it throws, and no row is written. The error line in this PR makes that refusal loud, with its cause in the meta.
  • Startup focus. A new guard is a new surface with no pull.

Tests

Run on head 377a12915e (after merging origin/main, which brought the step-7c rewrite of the same file):

  • pnpm --filter @objectstack/plugin-webhooks exec vitest run --maxWorkers=2: Test Files 21 passed (21), Tests 226 passed (226).

  • pnpm --filter @objectstack/plugin-webhooks typecheck: exit 0 (tsc --noEmit, tsconfig.scripts.json, check:test-typecheck: OK).

  • pnpm exec turbo run build --filter=@objectstack/plugin-webhooks...: 17 successful, 17 total.

  • New pins, in src/auto-enqueuer.test.ts under "a delivery that could not be recorded is reported at error":

    • the first loss is one error naming the webhook and sys_http_delivery, with the eventId and the cause in meta, and later losses go to debug, never warn;
    • two failing webhooks each get their own error;
    • a recorded delivery in between makes the next loss loud again;
    • the bulk path reports through the same register.

    They assert level, count and the named subjects, not prose.

Ablation (one leg, the changed behaviour)

node scripts/ablation-replace.mjs replaced this.writeFailing.add(sub.id); with the old bare warn and return. The anchor went from 1 hit to 0, and the blob from 81ae0a56ccfa to 17c85086162a. Then vitest run src/auto-enqueuer.test.ts gave Tests 4 failed | 40 passed (44): exactly the four new pins. The restore was git checkout HEAD. The restored blob is 81ae0a56ccfa, equal to the HEAD blob, and git diff HEAD is empty. This ran on head 377a12915e; the same leg on the pre-merge head 9a8a7f5103 read the same 4 failed and 40 passed. The subject is imported from source (./auto-enqueuer.js), so no dist/ was involved.

Gates

The local subset derived by node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack at 377a12915e has 70 families. Each exit was captured before any pipe.

  • 67 families exited 0. Among them: check:engine-double-contract, check:nul-bytes, check:doc-authoring, check:issue-citations, check:cross-package-test-inputs, check:test-source-alias, check:type-check-coverage, check:published-files, check:dts-closure, check:lean-entry-closure, check:adr-0087-registration --base origin/main, check:changeset-no-major and check:empty-changeset.
  • 2 families are NOT MEASURED: exit 3, PREREQUISITE NOT MET. check:dual-build-cjs-loads needs every package's dist/. check:i18n needs the CLI build closure. Neither reads anything this diff changes. CI's Lint & Repo Gates runs both.
  • 1 family is a declared narrowing, not run locally. check:type-check-debt (--re-measure) rebuilds the whole monorepo under the shared lock. CI's Lint & Repo Gates runs it.

--ran reconciliation: 70 derived, 67 run, 2 NOT-MEASURED, 1 UNRUN (the UNRUN is the declared check:type-check-debt). The full-battery Temporal Conformance (live PG + MySQL) job does not run on a PR. CI check-runs were not concluded when this body was written; the report on #24835 cites them.

Acceptance notes


Generated by Claude Code

…t error, once per webhook

A failed sys_http_delivery write loses the event for good (never sent,
retried or redeliverable) while the record write already answered 200 and
the webhook reads active. It was a bare WARN per event. It is now an error
said once per webhook, with the consequence and the fix, re-armed by the
next recorded delivery; later losses stay at debug with their eventId.

Claude-Session: https://claude.ai/code/session_011TdZ8GjixNZFLz1xJD9zzK
Co-authored-by: Claude <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/plugin-webhooks, touching 6 documentable anchor(s).

1 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/automation/webhooks.mdx (via AutoEnqueuer (symbol, a top-level class))
What this run could not see
  • the SDK route bridge reached 54 of 206 client-bound route-ledger rows — the other 152 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 152: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 55 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 97 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.
  • a key NAME is not a key, so the hand re-read the line above prescribes can land on the wrong schema. The same spelling is authorable on one governed type and a [REMOVED] tombstone on another for each of active, aria, joins, objects, template, tools and version (censused on [finding] tools is a key on BOTH AgentSchema (tombstoned, dead) and SkillSchema (live, cloud-attested), so a name-based search attributes skill examples to the agent key — it produced a false stop-the-line alarm on PR #19059 #19093 over the liveness ledger's governed types, top-level keys); nothing in a search result distinguishes the two, so a grep hit on a LIVE example reads as evidence about the DEAD key. Measured on fix(spec): the agent.tools liveness row says dead — it claimed live on a key the schema tombstoned #19059: content/docs/ai/agents.mdx was reported as contradicting the agent.tools tombstone over its tools: example at :161, which is inside the defineSkill({ block opened at :155 — the page was already correct. Settle ownership by PARSING the value against both schemas, never by the name: that literal PASSES SkillSchema, and as an AgentSchema it FAILS at tools with the tombstone prescription. ⛔ These names are not the whole class — a key retired through a .strict() guidance map leaves no tombstone in the walked shape and none of them here (tool.category, live as AIToolDefinition.category).

Coarse fallback — 4 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json df8bb40a032c016276e0b16c5edbd90cad086c9d → packageMentionDocs.

Which tree this was computed on

This run read content/docs from 37948a76aa8738e0045034030e5eee5270005312 — the merge of head 377a12915eebf50cb5b065858a711b7ddfa97d01 into base df8bb40a032c016276e0b16c5edbd90cad086c9d, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 37948a76aa8738e0045034030e5eee5270005312 && git checkout 37948a76aa8738e0045034030e5eee5270005312
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin df8bb40a032c016276e0b16c5edbd90cad086c9d 377a12915eebf50cb5b065858a711b7ddfa97d01 && git checkout -B drift-repro df8bb40a032c016276e0b16c5edbd90cad086c9d && git merge --no-ff 377a12915eebf50cb5b065858a711b7ddfa97d01

node scripts/docs-audit/affected-docs.mjs --json df8bb40a032c016276e0b16c5edbd90cad086c9d

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs df8bb40a032c016276e0b16c5edbd90cad086c9d → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

席位核对 · ACCEPT · domain:services seat 1 · 2026-10-20T11:50Z

os-justin · session_011TdZ8GjixNZFLz1xJD9zzK (seat post #6021). Reviewed against GitHub and origin/main 81120caac8, not against the report (os-dev-report 6289852046 on #24835). Head under review:

377a12915eebf50cb5b065858a711b7ddfa97d01
  1. Shape. Draft, base main, first line Part of #24835 (round 1: the log level), line-start Clause-②: no in the body and the changeset. 3 files, +187/−8, none on a governed surface (governedPathsIn answers []). The trial merge onto origin/main is clean. Every auto-enqueuer.ts hunk sits at line 680 or later, outside the step-7c region.

  2. The change, read in the code.

    • reportWriteFailure's ordinary branch used to log a bare warn per lost event. It now logs the first lost delivery per webhook at error. The line names the webhook and sys_http_delivery, says the event is lost and why nothing else shows it, and says where the cause is and what to restore.
    • Later losses for that webhook go to debug, each with its eventId.
    • A recorded delivery (.then(ok, …)) re-arms the report. A logger without error falls back to warn.
    • The ledger writeFailing holds webhook ids only, so it is bounded by the subscription count. The bulk path reports the same way. The parked-subscription branch is unchanged.
    • This is AGENTS.md's degradation rule: a dropped durable write is an error, said once, with the consequence and the fix.
  3. The premise, measured by the dev on a throwaway PostgreSQL 16 cluster with the real engine, SqlDriver, AutoEnqueuer and outbox.

    • One insert and one by-id update stored 1 delivery row on PG, against 2 on SQLite, together with the card's WARN.
    • Root cause: driver-sql's update() writes a knex Raw updated_at into the caller's patch, and ObjectQL publishes that patch as DataEvent.changes.
    • That is domain:engine's file, so round 1 stops short of it. Round 2 fixes it under claim 6289902041 with the cross-lane declaration 6289887999 on [PM seat] domain:server — ⏳ vacant #6367.
    • After this change, PG logged one ERROR, then one DEBUG for the second lost update.
  4. Pins and ablation. Four new cases in auto-enqueuer.test.ts:

    • one error on the first loss, naming the webhook and sys_http_delivery, with eventId and the cause in meta, and later losses at debug, never warn;
    • per webhook;
    • re-arm after a recorded delivery;
    • the bulk path.

    Ablation (the old bare warn back): 4 failed | 40 passed, exactly the new four. The restore was proven by blob equality.

  5. Changeset, @objectstack/plugin-webhooks patch, read sentence by sentence. "What was wrong", "What changed" and "Unchanged" each match 2. It states plainly that this change does not fix the PostgreSQL loss itself, which is fixed separately.

  6. Gates. At ACCEPT, every check-run on this head is success or an expected skip, among them TypeScript Type Check, Lint & Repo Gates, Test Core 1–6/6, Build Core and Governed Surface Queue Guard. Locally, 67 of 70 derived families exited 0:

    • 2 NOT MEASURED: check:dual-build-cjs-loads and check:i18n, each exit 3 on a missing prerequisite.
    • 1 declared narrowing: check:type-check-debt, whose --re-measure rebuilds the monorepo.

    CI's lanes measure all three. plugin-webhooks passed 226 of 226.

  7. The dev's fork: taken as option A, the producer fix in driver-sql, as round 2 of this card. Option B (snapshotting the event in objectql) would only add a second place that tolerates a producer defect.

  8. Accepted deviations.

    • Part of instead of Fixes: the loss is not fixed here.
    • Only the separable half was delivered under the hard stop.
    • The scratch PG cluster ran beside the worktree as postgres and was deleted.
    • The gate-runner incident: an overlapping runner was killed by PID and cwd, the partial dist was rebuilt, and the battery was re-run clean once.
    • One merge of main.
    • Model-free trailers.
  9. Out-of-scope dispositions.

    • Other readers of the mutated patch (afterUpdate hooks, realtime) go into round 2's producer fix.
    • The parked branch's error has no say-once: Acceptance notes.
    • check:durability-log-level judges try/catch only, and no new gate is added: Acceptance notes.
  10. Landing clause. Tier S: not governed, under the human-merge threshold. Next step: ready + auto-merge through the relay once every check on the current head is green or an expected skip. A push resets this reading.


Generated by Claude Code

@objectstack-fleet
objectstack-fleet Bot marked this pull request as ready for review October 20, 2026 11:51
@objectstack-fleet
objectstack-fleet Bot enabled auto-merge October 20, 2026 11:51
@objectstack-fleet
objectstack-fleet Bot added this pull request to the merge queue Oct 20, 2026
Merged via the queue into main with commit 3fbef41 Oct 20, 2026
42 checks passed
@objectstack-fleet
objectstack-fleet Bot deleted the claude/issue-24835-pg-webhook-enqueue-circular branch October 20, 2026 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants