fix: resolve suspected audit findings S1, S2, S4, S5 (S3 already fixed)#68
Merged
Conversation
…d by P17) Investigated all five suspected findings against current main; four confirmed and fixed, one already resolved. - S1 — bound the teardown LISTEN-connection close. A graceful close() on the same half-dead socket the health probe just flagged could block on the kernel keepalive and wedge the fetch loop. Now wait_for(close, _LISTEN_CLOSE_TIMEOUT) with a terminate() fallback; best-effort so teardown never raises. - S2 — detect partial-index predicate drift. A docker probe confirmed validate_schema() passes with a wrong timer_id_uq predicate (alembic's diff ignores postgresql_where), which then breaks the producer's ON CONFLICT arbiter. Added a pg_get_expr(indpred) probe comparing each partial index's predicate against the expected value. - S4 — guard the try_it_out import. It sat outside the resilience try/except, so an upstream module move/rename would break `import faststream_outbox` entirely. Moved inside the guard. - S5 — fix sync-mode batch ordering. Both fake batch paths dispatched each handler mid-feed (a handler saw a half-inserted batch) and emitted `published` last. Now insert the whole batch, emit `published`, then dispatch — mirroring production. - S3 — dismissed: already resolved by P17 (acked/nacked emit only after a successful flush; a lease-lost row emits lease_lost instead, never double-counted). Findings doc updated with the resolution. Full dockerized suite: 471 passed, 100% coverage; ruff + ty clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t indexes Review #1: the probe filtered `indpred IS NOT NULL`, so an expected index that exists but was created NON-partial (a plain UNIQUE (queue, timer_id) with no WHERE) was silently skipped — yet it breaks ON CONFLICT arbiter inference the same way a wrong predicate does, and alembic's diff can't distinguish it either. The probe now fetches all of the table's indexes (predicate NULL for non-partial) and, for each expected index that is present, flags both a NULL predicate ("not a partial index") and a wrong predicate. An absent index is still left to the alembic existence diff. This also makes the previously-untested branch (review #2) meaningful and covered. Adds test_validate_schema_detects_non_partial_index_on_present_index. Full dockerized suite: 472 passed, 100% coverage; ruff + ty clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
lesnik512
added a commit
that referenced
this pull request
Jun 13, 2026
Covers the post-0.9.0 work: suspected findings S1–S5 (#68), the P27 warning- attribution fix (#69), and the P29/P30 test-broker cleanup (#70). Patch bump — fixes/hardening only, no new breaking changes; the one opt-in behavior change is validate_schema() now catching partial-index predicate drift. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
Closes the audit's suspected findings (S1–S5) — the plausible-but-unproven ones. Each was investigated against current
main: 4 confirmed and fixed, 1 already resolved. Full dockerized suite: 471 passed, 100% coverage; ruff + ty clean.listen_conn.close()can hang on a half-dead socketwait_for(close, _LISTEN_CLOSE_TIMEOUT)with aterminate()fallback, best-effort so teardown never raises or wedges the fetch loop.validate_schema()misses partial-index predicate drifttimer_id_uqpredicate)_validate_index_predicates_sync— apg_get_expr(indpred, …)probe comparing each partial index'sWHEREagainst the expected value, since alembic's diff ignorespostgresql_where.processed_totaldouble-counts lease-lost rowsacked/nacked_*only after a successful flush; a lease-lost row emitslease_lostinstead. Pinned bytest_dispatch_one_lease_lost_emits_only_lease_lost_not_acked. No new code.try_it_outimport unguarded at top levelimportinside the resiliencetry/except, so an upstream module move/rename no longer breaksimport faststream_outbox.publishedorderpublished→ then dispatch, mirroring production (atomic batch INSERT → published → subscriber fetch). A handler no longer observes a half-inserted batch.How they were verified
timer_id_uq … WHERE timer_id IS NULL(wrong) passedvalidate_schema()— proving the gap — and the new probe now flags it. The integration test recreates that scenario.publishedis the first recorded event.Test Plan
ruff+tycleanThis completes the 2026-06-12 code audit: bugs (#61), test-holes (#66), improvements (#67), and now the suspected findings. Nothing from the audit's code-side remains open.
🤖 Generated with Claude Code