fix: post-11.07 audit — 8 findings (webhook keyset cursor, helm PG egress/PDB/selector, control-plane clock & retry, E4 settle window)#200
Merged
Conversation
…ghting Checks 3 and 4 broke the poll loop the instant one delivery/page appeared and then asserted count == 1, so a split delivery/page from the other pod landing a poll cycle (or one 60s alert tick) later passed undetected — a boundary the assertion could not distinguish from its absence (audit 2026-07-17 #3). Now: wait up to WAIT_SECONDS for the first emission, then dwell a settle window (DELIVERY_SETTLE_SECONDS=12; ALERT_SETTLE_SECONDS=75 > one 60s tick) and fail the moment a second delivery_id/page appears; assert == 1 after the dwell. bash -n clean; static contract test green. Not live-run (needs the scale kind stand). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…app clock record_webhook_delivery_outcome and schedule_outbox_retry wrote next_attempt_at with the application pod's datetime.now(UTC), but claim_due_webhook_deliveries / claim_due_outbox_entries compare it against Postgres's now(). Under NTP skew between app and DB hosts, this collapses the exponential backoff (app clock behind DB -> row immediately due -> retry burst) or over-delays it (app clock ahead). Mirror the lease-column pattern already used for lease_expires_at: compute the timestamp in SQL as now() + make_interval(secs => %s), passing the delay in seconds as a query parameter instead of a Python datetime. The NULL next_attempt_at cases (dead/failed rows) are unchanged in effect -- schedule_outbox_retry's NULL branch now flows through a SQL CASE keyed on the same retry_count >= max_retries condition instead of being computed in Python. The embedded (DuckDB) adapter is untouched: it is app-clock on both write and compare, which is internally consistent and not part of this bug. Audit finding #2 (2026-07-17 post-11 review).
…#8) A Dependabot `ignore` applies to both version and security updates — there is no per-ignore `applies-to` (that key is groups-only). The semver-major ignores on langchain-core/-text-splitters/langsmith/dagster therefore also suppress an auto security-update PR for a CVE fixed only in a major bump. That is an accepted tradeoff (their majors are gated deliberately) and not silent: the safety + pip-audit jobs still fail on such a CVE. Comment the tradeoff so it is a conscious decision, not a latent gap. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t DB errors If a webhook POST succeeds but record_webhook_delivery_outcome then hits a momentary DB blip (lost connection, exhausted pool checkout), the enqueue lease this call would have cleared is never cleared: the row stays pending+leased for the full claim_lease_seconds window (~300s) instead of a fast redrive, turning a ~2s recovery into a ~5min stall. Mirror record_api_usage's existing bounded-retry loop exactly: up to 3 attempts, catch _TRANSIENT_ERRORS (OperationalError, PoolTimeout), sleep 0.01 * (attempt + 1) between attempts, raise the last error if all attempts are exhausted. The mechanism is inline in record_api_usage (duplicated again in record_api_usage_batch, not a shared helper), so this is a third faithful copy rather than an invented pattern or a forced extraction. The retry wraps the whole method (fresh self._connect() per attempt) so all three outcome branches -- delivered, dead, pending-with-backoff -- get the retry, since a transient error on any of them would otherwise strand the lease. Each successful branch now returns explicitly from inside the try, which matters now that a bare fall-through would loop back for a spurious extra attempt. SQL text and parameters are unchanged from the finding-#2 commit; only re-wrapped for the new indentation. Does not change at-least-once semantics, lease duration, or embedded.py (the DuckDB adapter has no pool/network transient-error class to retry). Audit finding #4 (2026-07-17 post-11 review).
…audit #5 #6 #7) #5 NetworkPolicy: add a PostgreSQL :5432 egress rule (gated on controlPlane.store=postgres) + egressPorts.postgres. Without it, enabling networkPolicy on the postgres profile denies every pod's PG connect and the control-plane pool never opens. #6 PodDisruptionBudget: scope the selector to component=api so it guarantees a request-serving pod survives a drain; the component-less selector also matched worker pods, so one minAvailable:1 budget could let both API pods be evicted. Workers get no PDB by design (a single-replica worker PDB makes its node undrainable; the delivery loop resumes after reschedule from PG-claimed rows). #7 immutable selector: make app.kubernetes.io/component=api UNCONDITIONAL on the API Deployment (labels, spec.selector, pod template) instead of gating it on worker.enabled, so the immutable selector no longer changes when the split is toggled. The one-time migration for pre-existing worker.enabled=false installs (selector change needs a Deployment recreate) is documented in NOTES.txt and docs/migration/helm-component-selector.md; no pre-upgrade hook (it would fire on every upgrade, turning a one-time step into a recurring outage). Subagent-authored, reviewed and integrated by the main loop. Not live-verified against a cluster (no kind stand on this box). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ge the journal scan Audit 2026-07-17 finding #1 (latent P1). The webhook journal-scan cursor was second-granular: dispatch fetched `WHERE processed_at >= cursor ORDER BY processed_at, event_id ASC LIMIT scan_batch_size` and only ever advanced `_scan_cursor` to the whole second of the last handled row. If one second held >= scan_batch_size rows, every batch filled with that second's lowest-event_id rows, the cursor re-pinned to the SAME second, and every webhook for every tenant at/after that second was silently, permanently undelivered — with a healthy-looking cursor and no error. The comment stated the required invariant (batch > largest same-second cohort) but nothing enforced it, and no test covered it. Fix: make the scan cursor a composite (processed_at, event_id) keyset so it advances WITHIN a saturated second. - engine.fetch_pipeline_events: add `min_event_id`; when paired with `min_processed_at`, emit the keyset predicate `t > CAST(:ts) OR (t = CAST(:ts) AND event_id > :id)` (portable OR-decomposition — the row-value tuple form does not transpile to ClickHouse). Sub-second precision is preserved on this path so a saturated DuckDB second stays discriminable; ClickHouse processed_at is second-granular so the literal is a whole second there. `min_event_id`=None keeps the prior inclusive `>=` bound (search_index caller unchanged). - webhook_dispatcher: `_scan_cursor` becomes (processed_at, event_id) of the last CONTIGUOUS handled row; freeze-on-first-unseen/enqueue-failed retry semantics and the newest-row startup seeding are preserved. The seen-set is now a secondary safety net, not the primary dedup. Verified: OR-form transpiles to valid ClickHouse (parse duckdb -> generate clickhouse -> re-parse, table refs preserved) and drives a 2500-row saturated second to completion on DuckDB. New regression test seeds one second holding more than scan_batch_size rows and asserts every event is delivered and the cursor passes the second; it FAILS on the pre-fix code (confirmed by revert) and passes here. Live ClickHouse/Postgres not run in this environment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CHANGELOG [Unreleased]: one section for the 8 audit fixes (2 latent P1, 4 P2, 1 low), pointing at audit_2026-07-17_post11.md. serving-bridge #183 section: the webhook journal-scan cursor is now the composite (processed_at, event_id) keyset (strictly-after), not a second-granular inclusive high-water mark — documents why it advances within a saturated second and no longer wedges (audit #1). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
DORA Metrics
MTTR note: 4 failed run(s) are still unresolved in the selected window. |
CI's separate `ruff format --check` gate flagged them on PR #200; check and format are distinct gates and only the former was clean. 27 tests in the three files still pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…gression Two live-suite pieces, both verified against real servers via the Mac stand (ClickHouse 25.3, PostgreSQL 17.10 in Colima, SSH tunnels). 1) test_control_plane_postgres_live.py — 5 claim-path probes still encoded the pre-0669dd5 contract (fresh enqueue immediately claimable). Since 0669dd5 the enqueue winner stamps a claim lease on insert (it inline-delivers), so those probes got [] and failed. Main CI never caught it: the same session left three unformatted test files, lint failed, and test-integration was skipped on every push since — this branch's format fix un-masked the failures in PR #200 CI. Add _release_enqueue_lease() (hands rows to the claim path the way lease expiry would) and call it in the five probes; the probed properties — claim exclusivity, claim-lease invisibility, expiry redrive, restart survival, oldest-first — are unchanged. Before: 5 failed / 33 passed; after: 38 passed live. 2) test_webhook_keyset_clickhouse_live.py — the audit #1 residual: keyset was proven on DuckDB + transpile-shape only, never against a live ClickHouse. Three probes on a real server (own database, dropped after): the inclusive >= bound demonstrably cannot pass a saturated second (the wedge premise), the OR-decomposition keyset paginates through it, and the full dispatcher loop drains a 12-row second with scan_batch_size=5 — every event delivered exactly once, cursor past the second. 3 passed live; existing live CH suites still green (48 passed). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…sed seconds Adversarial review of the keyset fix (23106ff) found a P1 regression the strict cursor introduced into the ORDINARY load regime: ClickHouse processed_at is second-granular and event ids are UUIDs (not monotonic), so a cursor advanced into the still-open wall-clock second permanently excludes any same-second row that becomes visible later with a lower event_id. The old inclusive scan was lossless below scan_batch_size (it re-fetched the cursor second every poll; the seen-set deduped) and only wedged at >= 1000 rows/sec; the strict keyset never wedges but silently dropped late same-second arrivals at any load. The seen-set cannot save a dropped key: it prevents re-delivery, never a re-fetch below the frontier. Fix: every dispatcher journal fetch is bounded by a DB-clock settle watermark — processed_at <= CAST(now() AS TIMESTAMP) - INTERVAL 'N' SECOND (AGENTFLOW_WEBHOOK_SETTLE_SECONDS, default 3; 0 opts out, tests only) — so the frontier only crosses seconds no writer will stamp again. Both frames are self-consistent per backend (verified empirically: DuckDB stores tz-aware stamps in session-local wall time and CAST(now() AS TIMESTAMP) yields the same frame; ClickHouse stores and compares in server time). Startup seeding is bounded the same way: unsettled rows are deliberately not marked seen — they deliver once settled, deduped by the idempotent durable enqueue, so an event racing a restart is not lost. Re-fetching callers (search refresh, SSE, metric cache) keep the unbounded scan. Invariant the operator owns: settle > writer stamp-to-visibility lag + writer-DB clock skew. Tests: new unit regression drives the exact drop scenario (late lower-id same-second arrival behind an advanced frontier) — held back, then delivered exactly once, never lost; seeding pins settled-only; SQL-shape + transpile pins for the watermark clause; live-CH probe asserts a freshly stamped row is invisible to a settled fetch and visible to an unbounded one (passed against real ClickHouse 25.3). Fixture rows stamped NOW()/-1..3s in existing suites moved behind the watermark or opt out with settle=0 where the probe is mechanics, not freshness. Full unit suite: 1984 passed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ame in tests Re-review follow-ups on 983b9ac (no P0-P2 found; three P3 + two nits): - settle_seconds=0 now emits NO watermark clause at all. It previously rendered 'processed_at <= now() - INTERVAL 0 SECOND', which still withholds future-stamped rows under forward writer skew — not what the documented opt-out means. Pinned by a new SQL-shape test. - The two new dispatcher regressions now stamp journal rows by binding AWARE datetime.now(UTC) — exactly the production DuckDB write path — instead of pre-rendered naive-local strings, so the frame equivalence the watermark relies on (aware param and CAST(now() AS TIMESTAMP) both land session-local) is pinned by a committed test, not just by the empirical note in the previous commit message. - serving-bridge doc: the settle invariant is an operating requirement (a violation is silent); non-UTC DuckDB hosts see delayed (never dropped) delivery across the autumn DST fold. Known-accepted residuals (documented, not fixed): no runtime detector for a violated settle invariant; record_webhook_delivery_outcome retry can double-increment attempts if the connection drops exactly at commit-ack (narrow, premature dead-letter only). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
2 participants
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.
Summary
Lands all 8 findings from the post-11.07 adversarial audit (2 latent P1, 4 P2, 1 low, 1 docs), plus everything the landing itself surfaced: a main-inherited red format gate, a main-inherited PG-probe contract drift, and a P1 in the keyset fix found by pre-merge adversarial review.
store=postgres+networkPolicy.enabledcomponent: apiselector (one-time breaking upgrade; migration doc)next_attempt_atcomputed on the DB clockrecord_webhook_delivery_outcomeignorealso gates security-update PRs (docs)Landing follow-ups:
5252c3d— ruff format on 3 test files (the same files have kept main's lint red since 4a5d524, masking test-integration there).c6a81fa— 5 PG claim probes aligned with main's0669dd5enqueue-lease contract (38/38 on live PG 17.10) + new live-ClickHouse keyset regression (saturated second, exactly-once, on real CH 25.3 — also runs in CI's test-integration).983b9ac— settle watermark (pre-merge adversarial review found a P1 in the keyset fix itself: a strict cursor in the still-open second silently drops late-visible same-second rows with lower UUIDs — the old inclusive scan was lossless belowscan_batch_size). Every dispatcher fetch is now bounded to DB-clock-settled seconds (AGENTFLOW_WEBHOOK_SETTLE_SECONDS, default 3 s); startup seeding likewise. Operator invariant documented: settle > writer stamp-to-visibility lag + clock skew.7d58215— re-review follow-ups:settle=0is a true opt-out; the new regressions bind awaredatetime.now(UTC)exactly like production journal writes, pinning the frame equivalence.Verification
record_webhook_delivery_outcomeretry can double-incrementattemptsif the connection drops exactly at commit-ack (narrow; premature dead-letter only).🤖 Generated with Claude Code