Repository navigation
fix(effects): fence expired notification retries without a lease sweep (#81, #85) - #109
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
LIghtJUNction
left a comment
LIghtJUNction
left a comment
There was a problem hiding this comment.
Reviewed the final two-file diff and confirmed no temporary workflow remains. _live_attempt checks current state, attempt and stored deadline within the same metadata transaction as the completion/retry; expired/missing deadlines become uncertain, with no automatic resend, and superseded attempts are untouched. The 14 new tests and unchanged real PostgreSQL fixtures ran in the complete four-shard CI. Exact-head run 36351982270 is now success, including contracts, conformance and build. Scope remains the notification retry/completion paths; this does not claim all worker paths or the whole #81/#85 feature matrix are complete. Squash merge keeps temporary publication history out of main.
Refs #81 and #85. This fixes a newly reproduced notification-state gap; it does not close either umbrella.
Failure
_retry/_finish fenced state and attempt number, but did not independently check the stored lease deadline. If no second worker had swept the expired lease yet, an old callback could put the expired external attempt back into pending or mark it done. The existing regression only tested the already-swept case.
Fix
Share one current-attempt/deadline check across completion and retry. A matching expired or missing lease is transactionally recorded as uncertain with expired_execution_lease, preserving attempts/next_at. A superseded attempt or existing terminal state is left untouched. No automatic resend or late completion overwrites this decision. Live retry timing, limits, channel codes and authorization remain unchanged.
Verification
14 new tests cover mail/Webhook, exact/past/missing deadlines, retry/completion before any sweep, repeated callbacks, non-reclaimability and concurrent stale callbacks. Local reproduction with the explicitly test-only transactional SQLite adapter: 14 failed before, 14 passed after. This is not PostgreSQL evidence: the submitted tests use the unchanged real PostgreSQL installed fixture in the complete CI gate. No sender performs network I/O in these state tests.
Publication used a temporary feature-branch-only materializer because direct git connectivity is unavailable. Both input/output SHA-256 hashes were checked, the workflow deleted itself, and the remote effects.py Git blob exactly matches the tested file (1bb4eb3fa8bfdb7a7c0cee91eb116de0a3c936a4). Final diff must contain only source/tests; squash merge keeps staging history out of main. Full exact-head CI required. No production deployment or configuration/authority change.