refactor: address audit improvements P1–P35 (33 implemented, 2 deferred)#67
Merged
Conversation
From the 2026-06-12 code audit's improvements section. Each behavior change ships a regression test; the suite stays at 100% coverage. Producer / command validation: - P1 empty publish_batch validates the session type up front. - P2 a correlation_id kwarg that conflicts with headers['correlation_id'] now raises (was silently dropped); the kwarg otherwise wins. - P3 encode failures emit the published error metric (encode moved into the try). - P4 OutboxPublishCommand rejects batch-meaningless timer_id / correlation_id. - P5 queue validation (non-str / empty / >255) in the command constructor. - P6 documents the count=0 convention (error vs timer-conflict). SQL layer / schema: - P7 the identifier-length guard covers derived index/constraint names, not just the NOTIFY channel. - P8 CHECK ((acquired_token IS NULL) = (acquired_at IS NULL)) makes a half-set lease unrepresentable. - P9 timer_id flows into OutboxInnerMessage and the DLQ (column + CTE). - P10 delete_with_lease raises if given a dlq_payload with no dlq_table. - P11 fetch uses queue = ANY(:queues) for stable SQL / prepared-statement reuse. Fetch loop: - P12 positivity validation for fetch intervals / lease_ttl. - P13 reword the "min is a floor" claim (jitter is ±50% of the base). - P14 _on_notify wakes only for queues this subscriber serves. - P15 reword the misleading LISTEN-connection comment. - P16 stop() awaits the cancelled tasks before returning. Worker / dispatch: - P17 outcome metric emits only after a successful flush (lease-lost no longer double-counts); P34 dlq_written omits exception_type when there is none. - P18 a worker _OutboxConfigError is logged and swallowed, not routed through the connection-reconnect/backoff path. - P19 distinct log strings for the two failure sites; remove a dead `or` in _nack; clock-skew comment. Wiring / API: - P20 snapshot subscribers once in broker.stop(). - P21 the foreign-publisher warning names only the decorated subscriber's queues. - P22 a start()-time duplicate-queue check spanning routers. - P23 retry dataclass __post_init__ validation (jitter, delays, multiplier, caps). - P24 remove dead OutboxBrokerConfig.connect()/disconnect(). - P25 annotate _subscribers as the WeakSet it actually is. - P26 add the upstream-divergence marker to __aenter__. - P27 SKIPPED: no single static stacklevel is correct for both the direct and FastAPI-router call paths. Test broker / metrics: - P28 published error-status total fires at count=0. - P31 feed() rejects naive datetimes; P32 copies headers at fake boundaries; P33 documents fake-publish session leniency; P35 reword the "mirrors production" comment. - P29 + P30 DEFERRED: coupled test-infra changes (dedup FakeOutboxProducer vs _build_fake_publish*, and notify-on-feed which belongs in that shared helper); test-only, no behavior impact, better as a focused follow-up than risking the test broker the whole suite depends on. Full dockerized suite: 467 passed, 100% coverage; ruff + ty clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- P22: scope the start()-time duplicate-queue check to overlaps a router contributed to. Same-broker duplicates are already flagged at registration time, so this avoids warning twice for one mistake (review nit). - P14: cache the served queues as a frozenset for the NOTIFY-payload filter (O(1) membership, intent clearer) instead of scanning the queue list. P11 verified out-of-band: an EXPLAIN over a populated table confirms the `queue = ANY(:queues)` form still drives BitmapOr over both partial indexes (_pending_idx + _lease_idx) — no seq-scan regression. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
acked / nacked_retried / nacked_terminal now fire only after the terminal/retry write lands; a lease-lost row emits lease_lost instead (never a paired ack/nack). Make that explicit in the event-vocabulary doc so operators understand the post-P17 behavior change (raised in PR #67 review). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
lesnik512
added a commit
that referenced
this pull request
Jun 13, 2026
Release notes for the post-audit hardening release: B1–B16 bug fixes (#61), P1–P35 improvements (#67), T1–T8 test-quality guards (#66). Minor bump — new validation rejects some previously-accepted calls and the schema gains a CHECK constraint + DLQ timer_id column (migration required). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3 tasks
lesnik512
added a commit
that referenced
this pull request
Jun 13, 2026
…ings (#71) The 2026-06-12 code audit shipped across 0.9.0 (#61/#66/#67) and 0.9.1 (#68/#69/#70), but the invariant docs still described pre-fix behavior. Sync the drift the audit flagged plus the drift its own fixes introduced, and retire the now-resolved findings doc. CLAUDE.md / architecture: - relay: `_OutboxConfigError` no longer "rides AcknowledgementMiddleware to the nack path" — the worker catches it, logs at ERROR, and the row retries via lease expiry, not the retry_strategy (P18). Fixed in both CLAUDE.md and architecture/relay.md. - terminal-write API: `delete_with_lease(conn, …)` / `mark_pending_with_lease(conn, …)`, not the `*_with_conn` names. - NOTIFY: over-long table names now raise ValueError at construction (P7), not "silently lose NOTIFY and degrade to polling". - engine ownership: drop the phantom `EngineState` holder and the dead `OutboxBrokerConfig.disconnect()` (removed in P24). - schema: note the P8 `<table>_lease_ck` CHECK; validate_schema now also probes partial-index predicates (S2). - drain regression-test references point at the real test names (T8 added them to test_fake.py; integration covers the Postgres path). - timers NOTIFY-skip: "genuinely future-dated" wording (a past/zero activate_at/in does fire NOTIFY). Archive: - move planning/active/2026-06-12-code-audit-findings.md → planning/archived/ with status: shipped + PR/release provenance. - repoint the two release-notes "See also" links to archived/. Co-authored-by: Claude Opus 4.8 (1M context) <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
1 participant
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
Works through the Improvements section of the 2026-06-12 code audit (P1–P35). 33 implemented, each behavior change with a regression test; 2 deferred and 1 skipped, with reasons. Full dockerized suite: 467 passed, 100% coverage; ruff + ty clean.
Producer / command validation
publish_batchvalidates the session type up front.correlation_idkwarg conflicting withheaders['correlation_id']now raises (was silently dropped); the kwarg otherwise wins.publishederror metric (encode moved inside thetry).OutboxPublishCommandrejects batch-meaninglesstimer_id/correlation_id.queuevalidation (non-str / empty / >255) in the command constructor.count=0convention (error vs timer-conflict).SQL layer / schema
CHECK ((acquired_token IS NULL) = (acquired_at IS NULL))— a half-set lease is now unrepresentable.timer_idflows intoOutboxInnerMessageand the DLQ (column + CTE).delete_with_leaseraises if given adlq_payloadwith nodlq_table.queue = ANY(:queues)for stable SQL / prepared-statement reuse.Fetch loop
lease_ttl. · P13 reword the "min is a floor" claim. · P14_on_notifywakes only for served queues. · P15 reword the misleading LISTEN comment. · P16stop()awaits the cancelled tasks before returning.Worker / dispatch
dlq_writtenomitsexception_typewhen there's none)._OutboxConfigErroris logged + swallowed, not routed through the reconnect/backoff path. · P19 distinct log strings, dead-orremoved, clock-skew comment.Wiring / API
stop(). · P21 foreign-publisher warning names only the decorated queue. · P22 start()-time duplicate-queue check across routers. · P23 retry dataclass__post_init__validation. · P24 remove deadconnect()/disconnect(). · P25 annotate_subscribersas theWeakSetit is. · P26__aenter__divergence marker.Test broker / metrics
publishedtotal fires atcount=0. · P31feed()rejects naive datetimes. · P32 copy headers at fake boundaries. · P33 document fake-publish session leniency. · P35 reword the "mirrors production" comment.Deferred / skipped (with reasons)
FakeOutboxProducervs_build_fake_publish*; P30's notify-on-feed naturally belongs in the shared helper P29 would extract. Test-only, no behavior/correctness impact — better as a focused follow-up than risking the test broker the whole suite depends on.stacklevelis correct for both the direct@broker.subscriberpath and the FastAPI-router path (two extra frames).Test Plan
ruff format+ruff check+ty checkcleanFollow-up to #61 / #65 / #66. The audit's suspected findings (S1–S5) remain.
🤖 Generated with Claude Code