Skip to content

Fix pre-release review findings: dist AmiSSL guard, TLS handshake hangs, doc drift - #19

Merged
sidick merged 1 commit into
mainfrom
pre-release-review-fixes
Aug 30, 2026
Merged

Fix pre-release review findings: dist AmiSSL guard, TLS handshake hangs, doc drift#19
sidick merged 1 commit into
mainfrom
pre-release-review-fixes

Conversation

@sidick

@sidick sidick commented Aug 30, 2026

Copy link
Copy Markdown
Owner

Summary

Four independent reviewers (correctness, release mechanics, docs, Amiga-platform pitfalls) went over the repo ahead of the next tag. This fixes everything found except three items called out below as deliberately left for a decision/separate PR.

Release blockers (fixed + verified)

  1. make dist could silently ship a TLS-less mqtt.library. dist didn't fetch the AmiSSL SDK or require it - a fresh checkout (what release.yml's dist job does) would build without TLS while midge.readme advertises it as shipped. Fixed: dist now fetches the SDK if missing and re-invokes build as a sub-make (a plain prerequisite wouldn't work - M68K_HAS_AMISSL is fixed at outer-make parse time, before any recipe including the fetch runs), then hard-fails if the built library still lacks AmiSSL - a permanent mechanical guard, not a one-time patch. Verified: ran make dist for real from a clean .cache/ inside the CI container; confirmed the guard passes a real TLS build and fails a deliberately non-TLS one.
  2. The Amiga TLS handshake could hang a caller forever. No deadline on the handshake loop, and inside mqtt.library's connection subprocess nothing could abort it (break_sigmask wired up only after connect returns; wait_gate() treats a break wake as retry-me, not abort). A stalled peer (tarpit, wrong service on the port) wedged the caller with no recovery short of reboot. Fixed: 30s elapsed-time budget. Verified: library-tls-smoke and library-cafile-smoke both still pass end-to-end against real AmiSSL under Copperline.

Real defect (not release-blocking, fixed anyway)

  1. Host transport_openssl.c's SSL_connect() was single-shot. A blocking socket with 1s SO_RCVTIMEO can still return SSL_ERROR_WANT_READ on a timed-out read; one lost packet mid-handshake made mqtt_pub -s fail spuriously. Added the same bounded retry loop the AmiSSL transport already needed. Verified: broker-tls-smoke passes (verify-on, skip-verify, untrusted-cert-rejected).
  2. Issue scripts/verify-version.sh doesn't check MIDGE_VERSION_DATE #9: verify-version.sh now checks MIDGE_VERSION_DATE actually changed since the previous tag (plus format validation) - closes the gap where a release PR bumps the version but forgets the date. Verified against three simulated cases (real bump, stale-date bump, unchanged).

Docs drift (fixed)

  • CLI-Reference.md: ReadArgs template code blocks were missing TLS/TLSINSECURE/CAFILE even though the tables right below documented them; "two flags that only exist on the host builds" was stale twice over.
  • README.md: Status/Features still said TLS "is next" and pointed at docs/ARCHITECTURE.md's roadmap, which was deliberately trimmed and no longer covers post-v0.1 work.
  • mqtt-library.md: never mentioned mco_TLS/mco_TLSInsecure/mco_CAFile; error-code table was missing MQTTERR_STATE and mischaracterized MQTTERR_NOSTACK. Added a TLS section, including the one real CLI/library semantic difference worth flagging: mco_TLSInsecure does not imply mco_TLS the way the CLI's TLSINSECURE implies TLS.

Deliberately not changed

  • The actual version bump (0.1 → next) - that's its own release PR per CLAUDE.md.
  • run-dist: true on per-push CI - would've caught blocker Configure Renovate #1 earlier, but changes CI cost/cadence; a judgment call for you, not folded in here.
  • wait_gate()'s theoretical WaitSelect-failure misclassification and transport_openssl.c's low-confidence stale-errno edge case - both flagged by reviewers as pre-existing patterns / unconfirmed triggers, not concrete bugs.

Test plan

  • make test - 249/249 host unit tests pass.
  • make broker-smoke / make broker-tls-smoke - all pass, including the SSL_connect retry-loop fix.
  • make lint, make m68k, make test-target (real m68k codegen under Copperline) - all clean.
  • sh tests/library/tls-run.sh, sh tests/library/cafile-run.sh - full green on-target against real AmiSSL.
  • make dist from a clean .cache/ - real end-to-end run inside the CI container; AmiSSL-presence guard verified both ways (passes real build, fails a forced non-TLS build).
  • scripts/verify-version.sh - tested against a real bump, a stale-date bump (correctly fails), and an unchanged version (correctly passes).
  • mkdocs build --strict and docs2guide.py conversion both clean after the docs fixes.

🤖 Generated with Claude Code

https://claude.ai/code/session_01QWq1CRLCQy9AaD9UtLRSM9

@sidick @claude
Fix pre-release review findings: dist AmiSSL guard, TLS handshake han…
fabe995 
…gs, doc drift

Four independent reviewers (correctness, release mechanics, docs, Amiga
pitfalls) went over the repo before the next tag. Two release blockers
and several should-fixes, all fixed here and re-verified:

Release blockers:

- Makefile: `make dist` neither fetched the AmiSSL SDK nor required it,
  so a fresh checkout (exactly what release.yml's dist job does) would
  silently link a TLS-less mqtt.library while midge.readme advertises
  TLS as shipped. `dist` now fetches the SDK itself if missing and
  re-invokes `build` as a sub-make (M68K_HAS_AMISSL is a `:=`
  immediate-expansion variable, fixed at outer-make parse time - only a
  fresh sub-make process re-evaluates it against the now-populated
  .cache/), then hard-fails if the resulting mqtt.library still has no
  AmiSSL support - a permanent mechanical guard against this whole class
  of regression, not just a one-time fix. Verified: ran `make dist` for
  real inside the CI container from a clean .cache/, confirmed the
  AmiSSL-presence grep both passes on a real TLS build and correctly
  fails a deliberately non-TLS one.

- src/amiga/transport_amissl.c: the TLS handshake loop had no deadline
  and, inside mqtt.library's connection subprocess, no way to abort -
  break_sigmask is only wired up *after* connect() returns, and
  wait_gate() deliberately treats a break_sigmask wake as a retry, not
  an abort. A peer that accepts the TCP connect but stalls the handshake
  (tarpit, wrong service on the port, wedged broker) hung the caller
  forever with no recovery short of a reboot - MQTT_Disconnect()/
  MQTT_DeleteClient() couldn't reach the wedged child either. Added a
  30s elapsed-time budget (tool_now_ms()) to the loop. Re-verified both
  on-target smoke tests (library-tls-smoke, library-cafile-smoke) still
  pass end-to-end against real AmiSSL under Copperline.

Also fixed (real defect, not blocking):

- src/host/transport_openssl.c: a single SSL_connect() call on a
  blocking socket with a 1s SO_RCVTIMEO can return SSL_ERROR_WANT_READ
  when a read hits that timeout - blocking mode doesn't change what
  OpenSSL reports, only whether recv() itself blocks. One lost packet
  mid-handshake or a slow broker made `mqtt_pub -s` fail spuriously.
  Added the same bounded WANT_READ/WANT_WRITE retry loop (30s budget)
  transport_amissl.c already had to have. Re-verified: broker-tls-smoke
  passes (verify-on, skip-verify, and untrusted-cert-rejected cases).

- scripts/verify-version.sh (issue #9): now also checks
  MIDGE_VERSION_DATE actually changed since the previous release tag
  (format-validated too), closing the gap where a release PR bumping
  MIDGE_VERSION but forgetting the date would pass every existing check
  and ship binaries whose $VER carries the previous release's date.
  Verified against three cases: real bump (passes), version bumped with
  date left stale (fails with the new check), version unchanged
  (unaffected).

Docs drift:

- userdocs/CLI-Reference.md: the ReadArgs template code blocks were
  missing TLS/TLSINSECURE/CAFILE even though the argument tables right
  below them documented all three; "two flags that only exist on the
  host builds so far" was stale twice over (three flags, and no longer
  host-only since the Amiga transport shipped).
- README.md: the Status/Features sections still said TLS "is next" and
  pointed at docs/ARCHITECTURE.md's roadmap section, which was
  deliberately trimmed to shipped history and no longer covers
  post-v0.1 work (that's tracked via GitHub issues now, as the file
  itself says).
- userdocs/mqtt-library.md: never mentioned mco_TLS/mco_TLSInsecure/
  mco_CAFile at all; the error-code table was missing MQTTERR_STATE
  (-205) and mischaracterized MQTTERR_NOSTACK as something that can
  actually be returned (it's reserved - MQTT_CreateClient() returns a
  bare NULL on every creation failure). Added a TLS section, including
  the one real semantic difference from the CLI worth calling out:
  mco_TLSInsecure does NOT imply mco_TLS the way TLSINSECURE/-S implies
  TLS/-s on the CLI - it's simply ignored without mco_TLS set.

Deliberately not changed, for the user to decide:
- The actual version bump (0.1 -> next) - that's its own release PR per
  CLAUDE.md's process, not something to fold into a fixes PR.
- Enabling `run-dist: true` in ci.yml's per-push CI (would have caught
  the dist blocker earlier, but changes CI cost/cadence - a judgment
  call, not a fix).
- wait_gate()'s theoretical WaitSelect-failure misclassification and
  transport_openssl.c's low-confidence stale-errno edge case in
  openssl_recv() - both flagged as pre-existing patterns/unconfirmed
  triggers by the reviewers themselves, not concrete bugs.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QWq1CRLCQy9AaD9UtLRSM9
@sidick
sidick merged commit 1739d05 into main Aug 30, 2026
11 checks passed
@sidick
sidick deleted the pre-release-review-fixes branch August 30, 2026 12:33
This was referenced Aug 30, 2026
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@sidick