fix(hardening): 0.4.2 payload - bounded migrate lock, stamp --force, asyncpg translation, per-file lock_timeout - #5
Merged
Conversation
_chain_session used a blocking pg_advisory_lock: a hung or stuck holder (wedged txn, leaked session) would hang every migrate/stamp indefinitely. Mirror the executor's with_advisory_lock shape inside the chain session: pg_try_advisory_lock polled every 0.5s against a time.monotonic() deadline (default 30s), SqlpushError on exhaustion, negative-wait validation before any connection attempt. Plumb advisory_wait through api.migrate and add the --advisory-wait flag to the migrate verb (same default and style as push). The shared advisory_key import and the versions-DDL/yield/unlock structure are untouched, so the migrate/push interlock is preserved.
run_stamp upserted unconditionally and never read recorded state, so stamping after editing an applied file silently refreshed its checksum — wiping the edit-detection integrity migrate gates on. Read the recorded checksums first; a registered-but-different checksum raises an instructive SqlpushError (file edited after apply; pass force/--force to accept the new content) and the first mismatch stops the walk, so nothing after it registers. With no previous row or force=True the upsert is unchanged. Plumb force through api.stamp, add the --force flag to the stamp verb, refresh the run_stamp docstring, and document stamp in the CLI exit-code block (0 registered / 1 blocked or refused via the typed-error path).
… mkdir error
run_migrate's per-file transaction ran without any lock timeout, so a
chain file whose DDL got stuck behind another transaction's lock
queued indefinitely. Issue SET LOCAL lock_timeout inside each file's
txn — txn-scoped so it dies with the file's commit/rollback, same
inline-int style and rationale as push's transactional segment
(executor.py; PG rejects bind params for SET). Plumb lock_timeout
(5.0 default, negative rejected before any file/connection work)
through api.migrate and add the --lock-timeout flag to the migrate
verb, matching push.
Also type revision()'s output-directory creation: a bare mkdir
OSError now surfaces as SqlpushError ("cannot create migrations
directory ..."), same typing as the adjacent write_text wrap — no
behavior change in the happy path, so no changelog entry.
_sync_engine_from resolved DSN strings and AsyncEngines verbatim into a SYNC engine; a postgresql+asyncpg URL renders fine but its dialect is async-only, so migrate/stamp/ensure_schema failed at connect. Both branches now route through _translate_asyncpg (URL API, driver swap only — host/db/credentials/query preserved); psycopg is a runtime dep so the translated engine connects, asyncpg never has to be installed.
_dedup_enum_types' slow path always reconstructed a PlannedOperation even when every statement survived (kept == stmts). Track a dropped flag and append the original object (identity) when nothing was removed; only ops that actually lost a duplicate statement are rebuilt. Behavior otherwise identical — the rebuild round-trips to byte-equal SQL for undropped renders, so plan output is unchanged. The identity test is DB-free: test_enum_dedup.py drops its module-level pg mark (marks now live on the DB tests individually) so the unit test never requires the server.
Two coverage gaps in the S1/S2 hardening: - push-path enum dedup: the shared-native-enum dedup was pinned at plan level and on the migrate replay path only. An end-to-end api.push test proves the dedup survives the apply path (advisory-lock re-plan included): the push applies cleanly and pg_type holds exactly one row for the shared enum. - S1 parity boundary: only the m_cols == r_cols (equal) case was pinned. Mirror the born-DESC construction with a same-named declared index on a DIFFERENT column: the drop+add pair must still surface as drift — owner match alone must not prune. Mutation-checked: forcing the prune regardless of columns fails this test while the equal-case twin stays green.
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
Release-hardening cycle implementing the six fixes adjudicated in the gap analysis:
3612b51):_chain_sessionnow mirrorswith_advisory_lock(pg_try_advisory_lock+ 0.5s poll + monotonic deadline, default 30s) instead of blocking forever onpg_advisory_lock. Exposed asapi.migrate(advisory_wait=...)and--advisory-wait; stamp rides the same bounded default. The push/migrate interlock (sharedadvisory_key) is preserved.--force(a7ebb43): stamping a chain file whose recorded checksum no longer matches (edited after apply) now fails loud with an instructive error instead of silently refreshing the checksum and wiping edit-detection. First mismatch stops the walk; nothing after it registers.force=True/--forceaccepts the new content deliberately.a93ca95): each migrate file transaction opens withSET LOCAL lock_timeout(txn-scoped, matching push's executor), exposed aslock_timeout/--lock-timeout(default 5s). Same commit types therevision()mkdir failure asSqlpushError.8c95049): the sync facade (_sync_engine_from) translatespostgresql+asyncpgURLs topostgresql+psycopg(URL-API driver swap, credentials preserved) instead of dying on the async-only driver. asyncpg never needs to be installed.da2927e):_dedup_enum_typesreturns the original op when nothing was dropped (output byte-identical, object identity preserved).84064c3): end-to-end push-path enum dedup test (singlepg_typerow,check().clean) and the TimescaleDB index-parity boundary (same name, different columns still reports drift as the drop+add pair). Both proven non-vacuous by mutation checks.Verification
144 passed, 1 xfailed(baseline 131 + 13 new) against livetimescaledb-ha:pg17; DB-free subset skips cleanlyruff check,ruff format --check,ty checkall cleanRelease surface
CHANGELOG
[Unreleased]carries four Fixed bullets (B3, B4, S2, B2 — API and CLI surfaces named in each); CLI exit-code docstring now documents stamp/revision. This PR is the 0.4.2 payload: on merge, cutv0.4.2.