Skip to content

fix(django): close automatically opened client after send failures - #39

Open
Kayvan-Zahiri wants to merge 1 commit into
ActiveCampaign:mainfrom
Kayvan-Zahiri:codex/close-django-client-after-send-failure
Open

Kayvan-Zahiri wants to merge 1 commit into
ActiveCampaign:mainfrom
Kayvan-Zahiri:codex/close-django-client-after-send-failure

Conversation

@Kayvan-Zahiri

Copy link
Copy Markdown

Problem and change

Django's Postmark backend leaves an automatically opened client attached after send_batch raises or returns a failed message. The close call is only reached after every batch succeeds.

Put the batch loop inside try/finally so clients opened by send_messages are closed on failure too. Keep this cleanup inside the existing exception-handling boundary so fail_silently, error signals, partial success counts, and caller-owned connection reuse remain intact. Add an Unreleased changelog entry.

Fixes #38.

Validation

  • Before: 4 failed, 4 passed new lifecycle cases. Cover transport exceptions and per-message failures, both fail_silently values, and automatically opened versus caller-owned clients.
  • After: full pytest --cov=postmark --cov-report=term --cov-fail-under=85 — 462 passed, 96.32% coverage.
  • Full Ruff lint and format checks pass; MyPy passes across 64 source files.
  • Required pre-commit run --all-files — all applicable hooks pass, including Poetry metadata/lock consistency, pinned Ruff/MyPy, codespell, Bandit, and secret scanning.
  • git diff --check passes. No unrelated file changes.
  • Python 3.12 / Django 5.2; full Python/Django matrix not run. The existing fork-in-multithreaded-process test emits a deprecation warning.
  • Synthetic messages and mocked clients only. No hosted Postmark requests, actual email delivery, credentials, paid resources, or UI automation.

AI assistance: OpenAI Codex investigated, implemented, tested, and prepared this draft. Human verification is not claimed.

@gregsvo

gregsvo commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator

Excellent. Good catch. I'm pulling/testing now. Will merge if no issue.

@gregsvo

gregsvo commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator

Excellent work here, @Kayvan-Zahiri. I checked it out all tests pass. lint and mypy are clean, all good.

I did find one small edge case: if client.close() itself raises while we're cleaning up after a failed send, the close error replaces the original send error, both for the caller and for the on_exception signal. Obviously I'd like the original error to win. I want to get this out fast, and since you've allowed maintainer edits, I'm pushing one small follow-up commit to your branch (guard, and 2 tests). Your commit stays exactly as it is and remains the one that fixes the issue.

Could you also mark the PR Ready for review? Our CI needs a maintainer to approve the first run on a fork, and I'll do that as soon as it's ready. This should go out in the next release, and I'll credit you in the release notes. Thanks again!

@gregsvo gregsvo self-assigned this Oct 28, 2026
@Kayvan-Zahiri
Kayvan-Zahiri marked this pull request as ready for review October 28, 2026 21:53
@Kayvan-Zahiri

Copy link
Copy Markdown
Author

Marked ready. Thanks for testing it, and good call on letting the original send error win over a close() failure.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Django backend retains automatically opened client after send failure

2 participants