CI follow-up: "pg_tle install path" job fixed in 313a87c.

Cause: in the pg_tle job, pgque is installed as an extension. tests/test_uninstall_guard.sql executed sql/pgque-tle-uninstall.sql mid-suite, and after the C4/C9 fixes the script now (correctly) stops pgque, drops the extension -- taking the whole pgque schema with it -- and unregisters from pg_tle. The test's "plain (non-extension) install must survive" assertion then failed because the install was never plain. The assumption only holds in the plain-install jobs.

Fix: gate the two sub-tests that execute sql/pgque-tle-uninstall.sql behind a psql-level \if on pg_catalog.pg_extension not containing pgque (psql-level because the script is run via \i, which a DO-block skip cannot prevent). When pgque is an extension, the suite emits the usual SKIP: notice and the script is never executed; the pgque.stop() save/restore around the suite is unaffected. Test 1 (sql/pgque_uninstall.sql aborts when stop() raises) is safe under both install modes and stays ungated. The extension path of the TLE uninstall script remains covered by tests/test_tle_install.sql.

Verification (local, plain install, PG 16):

• fresh scratch DB + psql -v ON_ERROR_STOP=1 -f sql/pgque.sql, then full tests/run_all.sql: === ALL TESTS PASSED === with all three guard sub-tests exercised (PASS notices present, no skips).
• skip branch simulated (pg_tle is not installable locally) by forcing the gate variable to true in a temp copy: SKIP: notice emitted, no line of pgque-tle-uninstall.sql executed, schema intact, stop() restored, exit 0.

https://claude.ai/code/session_01KAaEGkQZmey1D1xCsVGmqv

Generated by Claude Code

samorev Code Review Report

• PR: fix: harden uninstall paths (stop jobs, narrow guards) #290 - fix: harden uninstall paths (stop jobs, narrow guards)
• Author: @NikolayS
• AI-Assisted: Unknown

BLOCKING ISSUES (1)

HIGH MR/PR state - Review target is draft

The review target is still marked as draft.
Fix: Mark it ready for review before merge.

Summary

Note:

• Findings: High-confidence issues (8-10/10) - blocking or non-blocking per severity
• Potential: Medium-confidence issues (4-7/10) - review manually
• Filtered: Low-confidence issues (0-3/10) - excluded as likely false positives

provider=github
kind=pr
project=NikolayS/PgQue
number=290
target=github:NikolayS/PgQue#290
state=OPEN
draft=true
diff_lines=329
diff_added=207
diff_removed=7
diff_bytes=13578
comments_count=1
commits_count=4
ci_status=success
ci_summary=total=14 success=14 failure=0 pending=0 other=0
prompt=.claude/commands/review-mr.md
blocking=false
posted_by=gh
no_comment=false
live_posting=posted

samorev-assisted review (AI analysis by Tanya301/samorev)

PR-lifecycle steps 1–3 — evidence

Environment: local PostgreSQL 18.3, superuser, pg_cron preloaded (shared_preload_libraries = pg_stat_statements,pg_cron, cron.database_name = pgque_test). pg_tle not available locally — the TLE extension-refusal assertion (C9) is covered by the pg_tle install path CI job, and the guard test degrades cleanly without it.

1. CI green

All 14 checks pass on HEAD (313a87c). mergeStateStatus = CLEAN, mergeable = MERGEABLE, not a draft.

2. Diff review + generated-file consistency

Reviewed lifecycle.sql, both uninstall scripts, build/transform.sh, and the two generated files. Ran the transform and confirmed zero drift:

$ bash build/transform.sh && git diff --exit-code -- sql/pgque.sql sql/pgque-tle.sql
GENERATED FILES CLEAN (no drift)

The narrowed handler (when undefined_function or invalid_schema_name) and the same-do-block drop are present in both scripts; pgque.uninstall() gains the pg_extension guard; the TLE header no longer claims non-psql compatibility.

3. Real testing with pg_cron

Happy path (PR-290 sql/pgque_uninstall.sql): install → pgque.start() schedules 4 cron jobs → uninstall removes all 4 jobs and the schema.

cron.job after start(): pgque_ticker, pgque_retry_events, pgque_maint, pgque_rotate_step2
-> run sql/pgque_uninstall.sql (exit 0)
cron.job after uninstall: (empty)
pgque schema after uninstall: (gone)

The bug this PR fixes — orphaned jobs when stop() fails. With stop() replaced by a raiser (simulating a real cron.unschedule permission error / untrusted-pg_timetable-owner exception), comparing main vs this branch:

## MAIN, stop() raises:
DROP SCHEMA
schema after: <gone>
ORPHANED jobs: pgque_ticker, pgque_retry_events, pgque_maint, pgque_rotate_step2

## PR-290, stop() raises:
ERROR:  simulated cron.unschedule permission failure
schema after: pgque        <- survives
jobs after:  pgque_ticker, pgque_retry_events, pgque_maint, pgque_rotate_step2

Idempotency: PR pgque_uninstall.sql on a DB with no pgque schema → exit 0 (narrowed handler tolerates "not installed", drop schema if exists no-op).

Full suite on HEAD (fresh install in pgque_test):

$ psql -v ON_ERROR_STOP=1 -f sql/pgque.sql        # exit 0
$ psql -v ON_ERROR_STOP=1 -f tests/run_all.sql    # exit 0
162 PASS
=== ALL TESTS PASSED ===

test_uninstall_guard (new) passes all four assertions — abort-before-drop for both scripts, stop()-called-before-drop-extension, and the real stop() restored afterward. (The ERROR: simulated stop() failure lines in the log are the test's intentional red-phase simulations, each immediately followed by its PASS.)

Verdict

MERGE-READY per steps 1–3. Note (non-blocking): merges after 292 → 287; the shared generated files (sql/pgque.sql, sql/pgque-tle.sql) will likely need a mechanical bash build/transform.sh rebase in sequence.

Deep review — uninstall hardening (steps 1–3, post-rebase)

Reviewed manually and end-to-end (the automated review bot's report for this PR contained no code analysis, so I treated the PR as unreviewed). Rebased once onto current origin/main (a4bd61c, which now carries 292/287/267), regenerated with bash build/transform.sh, force-pushed (d862ba5). Diff vs main is unchanged in scope: 9 files, +207/−14. CI is green on d862ba5 (15/15).

Verdict

No blocking findings. The four fixes are correct and behave as claimed against a real pg_cron. Details and lower-severity notes below.

What I verified (correctness)

• Job unscheduling, pg_cron: pgque.start() schedules 4 jobs (pgque_ticker, pgque_retry_events, pgque_maint, pgque_rotate_step2); the PR's sql/pgque_uninstall.sql removes all 4 and drops the schema (exit 0). Idempotent rerun on the now-empty DB: exit 0.
• Job unscheduling, pg_timetable: stop() (lifecycle.sql:199-202) delegates to stop_timetable() when config.scheduler = 'pg_timetable', and stop_timetable() deletes all 4 jobs by name. The uninstall function pgque.uninstall() also calls both stop() and stop_timetable() unconditionally (lifecycle.sql:432-438), so both schedulers are covered. (pg_timetable exercised by the green pg_timetable real worker CI job, not locally.)
• Abort-before-drop / no swallowing (C6): with stop() forced to raise, sql/pgque_uninstall.sql (same-do-block drop, pgque_uninstall.sql:12-20) aborts before the drop schema — schema and all 4 cron jobs survive. On main the old exception when others then null swallowed the failure and dropped the schema anyway, orphaning all 4 jobs. pgque.uninstall() gets the same guarantee for free: a raising stop() propagates out of the function before drop schema pgque cascade (lifecycle.sql:440).
• Untrusted-owner failure is correctly NOT swallowed: stop_timetable() raises raise_exception (P0001) for an untrusted pg_timetable schema/API owner (lifecycle.sql:390,401). The narrowed handler catches only undefined_function/invalid_schema_name, so this real failure propagates and aborts the uninstall — exactly the C6 intent, and it holds for the pg_timetable failure mode too, not just cron.
• Guard narrowing does NOT block legitimate uninstalls: a working stop() raises nothing, so the drop proceeds normally; the pg_extension-membership guard in uninstall() (lifecycle.sql:427-431) fires only for extension installs (correct — those must use drop extension), and plain schema installs are unaffected. Confirmed by the full suite (165 PASS, ALL TESTS PASSED) plus test_tle_install.sql's refusal assertion.
• TLE header (C7): the "psql script / call pgtle.install_extension() from other clients" text is present in both build/transform.sh and the generated sql/pgque-tle.sql:11-13.
• Generated-file consistency: bash build/transform.sh after rebase produces zero drift in sql/pgque.sql and sql/pgque-tle.sql.

Findings

MINOR (pre-existing, out of this PR's diff — recommend follow-up, not a blocker for 290)

• sql/pgque-additions/lifecycle.sql:221-233 — inside stop(), the by-name unschedules of pgque_retry_events and pgque_rotate_step2 are each wrapped in exception when others then raise notice. This is the same swallow-a-real-failure pattern this PR removes from the caller: a genuine cron.unschedule failure on those two jobs (e.g. a permission error) would be downgraded to a NOTICE and those two jobs would orphan, even though the ticker/maint unschedules (by id, unguarded) would have already aborted or succeeded. Because start() stores only ticker/maint ids in config, these two must be unscheduled by name — but the handler could be narrowed to the "job does not exist" case (cron.unschedule raises when the name is absent) rather than when others. Not introduced here; flagging because it's the same bug class and leaves a residual orphan path the PR's title implies is closed.

NIT

• Narrowed handler matches by sqlstate regardless of cause (pgque_uninstall.sql:15, pgque-tle-uninstall.sql:24-26): if an installed pgque.stop() happened to raise 42883/3F000 for some reason other than "not installed," it would still be swallowed. stop() is simple enough that this window is negligible, and it's a large improvement over when others. No change needed; noting the semantics.
• Script vs. function asymmetry: sql/pgque_uninstall.sql calls only stop() (relying on config.scheduler to route to stop_timetable()), whereas pgque.uninstall() calls both unconditionally. Correct in practice because start()/start_timetable() each clear the other scheduler first, so config.scheduler is authoritative — but the two paths aren't identical.
• Test-coverage gap: test_uninstall_guard.sql uses instrumented stop() fakes, so it does not assert (a) idempotent rerun tolerance on a fresh/uninstalled DB, or (b) real-pg_cron end-to-end job removal by the script. Both are covered by my manual run above and the pg_cron install path CI job; a small assertion for (a) would lock in the C6 narrowing against future regressions.

Testing evidence

Local PG 18.3, pg_cron preloaded (cron.database_name = pgque_test), superuser. Happy path, forced-failure abort, idempotent rerun, and full suite all pass on the rebased head d862ba5; details in the prior comment on this PR.

samorev Code Review Report

• PR: fix: harden uninstall paths (stop jobs, narrow guards) #290 - fix: harden uninstall paths (stop jobs, narrow guards)
• Author: @NikolayS
• AI-Assisted: Yes

This report supersedes the earlier samorev review --fetch comment on this PR. That comment was a CI/draft gate check only and performed no code analysis (it hardcoded zeros in every area). The results below come from a genuine per-area review of the diff, verified end-to-end against a real pg_cron on PostgreSQL 18.3.

No blocking issues. The four fixes are correct and behave as claimed; two non-blocking notes and two lower-confidence items below.

What was verified

• Abort-before-drop (no swallowing): with stop() forced to raise, sql/pgque_uninstall.sql aborts before drop schema — schema and all four cron jobs survive. The prior exception when others then null swallowed the failure and dropped anyway, orphaning jobs. pgque.uninstall() gets the same guarantee: a raising stop() propagates before drop schema pgque cascade.
• Untrusted-owner failure correctly NOT swallowed: stop_timetable() raises P0001 for an untrusted pg_timetable owner; the narrowed handler catches only undefined_function/invalid_schema_name, so this real failure propagates and aborts the uninstall.
• Guard narrowing doesn't block legitimate uninstalls: a working stop() raises nothing so the drop proceeds; the pg_extension-membership guard fires only for extension installs. Full suite 165 PASS + test_tle_install.sql refusal assertion.
• Job unscheduling: happy-path removes all four jobs (pgque_ticker, pgque_retry_events, pgque_maint, pgque_rotate_step2) and drops the schema; idempotent rerun on the empty DB exits 0.
• Generated-file consistency: bash build/transform.sh after rebase produces zero drift; TLE header text present in both build/transform.sh and sql/pgque-tle.sql.

NON-BLOCKING (2)

LOW sql/pgque-additions/lifecycle.sql:221-233 - residual swallow inside stop() (pre-existing, out of this PR's diff)

The by-name unschedules of pgque_retry_events and pgque_rotate_step2 are each wrapped in exception when others then raise notice — the same swallow-a-real-failure pattern this PR removes from the caller. A genuine cron.unschedule permission error on those two jobs would be downgraded to a NOTICE and the jobs would orphan.
Suggestion: Narrow the handler to the "job does not exist" case rather than when others. Not introduced here, but it's the same bug class and leaves a residual orphan path the PR title implies is closed. Recommend a follow-up.

LOW tests/test_uninstall_guard.sql - coverage gap

Uses instrumented stop() fakes, so it does not assert (a) idempotent-rerun tolerance on an uninstalled DB, or (b) real-pg_cron end-to-end job removal by the script. Both are covered by the manual run + the pg_cron install path CI job, but not by a committed regression.
Suggestion: A small assertion for (a) would lock in the C6 narrowing against future regressions.

POTENTIAL ISSUES (2)

LOW sql/pgque_uninstall.sql:15 - narrowed handler matches by sqlstate regardless of cause (confidence: 5/10)

If an installed pgque.stop() raised 42883/3F000 for some reason other than "not installed", it would still be swallowed.
Suggestion: stop() is simple enough that this window is negligible, and it is a large improvement over when others. Noting the semantics; no change needed.

LOW sql/pgque_uninstall.sql vs lifecycle.sql:432-438 - script/function asymmetry (confidence: 5/10)

The script calls only stop() (relying on config.scheduler to route to stop_timetable()), whereas pgque.uninstall() calls both unconditionally.
Suggestion: Correct in practice — start()/start_timetable() each clear the other scheduler first, so config.scheduler is authoritative — but the two paths aren't identical. Consider aligning them.

Summary

Note:

• Findings: High-confidence issues (8-10/10) - blocking or non-blocking per severity
• Potential: Medium-confidence issues (4-7/10) - review manually
• Filtered: Low-confidence issues (0-3/10) - excluded as likely false positives

provider=github
kind=pr
project=NikolayS/pgque
number=290
target=github:NikolayS/pgque#290
state=MERGED
draft=false
diff_lines=329
diff_added=207
diff_removed=7
diff_bytes=13596
comments_count=4
commits_count=4
ci_status=success
ci_summary=total=15 success=15 failure=0 pending=0 other=0
prompt=.claude/commands/review-mr.md
blocking=false
posted_by=gh
no_comment=false
live_posting=posted

samorev-assisted review (AI analysis by Tanya301/samorev)