Skip to content

1.0 hardening: fix @transaction early-return composition - #7

Merged
quinnj merged 5 commits into
mainfrom
release-1.0-hardening
Aug 7, 2026
Merged

1.0 hardening: fix @transaction early-return composition#7
quinnj merged 5 commits into
mainfrom
release-1.0-hardening

Conversation

@quinnj

@quinnj quinnj commented Aug 7, 2026

Copy link
Copy Markdown
Member

Continuation of the 1.0 readiness work from #5, on a fresh branch against merged main: independent adversarial review rounds against a live PostgreSQL, fixing what they surface, until a round comes back clean.

The problem: @transaction early-return was unsound under composition

Round 13 (the first review of #5's merged tail) found that the early-return support rewrote return x into an untagged thrown marker, so the dynamically nearest @transaction expansion always intercepted it. Verified live: nested @transaction + return silently rolled back all levels and left the connection stuck in a transaction; a user try/catch swallowed the marker and returned its own fallback value; return inside task macros threw the marker instead of producing the task's value; break/continue left the transaction open.

The evolution of the fix (rounds 14–16 + external review)

Commits 1–4 fixed this incrementally: per-expansion tokens, guards injected into user catches, and a growing skip list of closure/task-forming constructs (short-form defs, @spawnat, @fetch/@fetchfrom, comprehensions). Each round's reviewer found the next hole in the allowlist.

External review (codex) then proved the endpoint of that trajectory: no finite allowlist can cover third-party task macros (reproduced with a minimal @local_task), and — the key insight — the expansion's finally already gives plain return the intended semantics with no rewriting at all.

Final design (commit 5, net −140 lines)

The marker struct, AST walker, try-guard injection, and both skip lists are deleted. The macro is now just try/catch/finally:

  • Every non-exceptional exit commits — normal completion, return, break, continue. A return unwinds through every enclosing expansion's finally, each committing its level exactly once, innermost first. Only a thrown exception rolls back.
  • Plain Julia semantics everywhere: user catches cannot intercept a plain return; closures, do-blocks, comprehensions, and any task-forming macro (standard or third-party) keep their ordinary meaning untouched.
  • The finally handles its own commit failure (the second P1 from external review): it rolls back the current level before propagating — commit at savepoint depth leaves the depth unchanged on failure, so each enclosing level unwinds its own. Previously a RELEASE SAVEPOINT failure during a break out of an aborted nested level escaped cleanup and left the outer transaction open with its work pending.

Verification

  • Behavioral regressions for every failure mode found along the way: nested return (incl. cross-connection and 3-level), both user-catch shapes, Threads.@spawn, a third-party @local_task macro, Distributed.@spawnat/@fetch/@fetchfrom (run locally on worker 1), short-form helpers escaping the block, break/continue, recursion re-entering the same expansion, plain-vs-wrapped flattened-iterator equivalence, and the nested-break-with-aborted-savepoint commit-failure case (asserts the server error surfaces and nothing stays open client- or server-side).
  • Mutation-verified: restoring the original marker behavior fails 6+ tests; removing the finally rollback fails 5.
  • Suite: 1433/1433 locally (Docker integration + TLS fixture); full 18-check CI matrix green on every commit.
  • All three external review threads answered on their respective conversations.

🤖 Generated with Claude Code

The return-rewrite threw an untagged TransactionReturn marker, so the
dynamically nearest @transaction expansion always intercepted it:

- a return inside a NESTED @transaction committed only the inner savepoint,
  and the inner expansion's own plain `return` then skipped every enclosing
  commit — all levels' work was silently rolled back and the connection was
  left inside the outer transaction
- a user try/catch inside the body swallowed the marker and returned the
  catch's value instead of the intended return value, silently
- a return inside Threads.@Spawn / @async in the body was rewritten too, so
  the task threw the marker instead of producing its value

Each expansion now tags its markers with a compile-time token. A catch that
receives a foreign marker commits its own level and keeps unwinding to the
owning expansion, so an early return commits every enclosing level and
returns exactly once. User catch blocks get a guard injected that rethrows
the marker (a private type no handler can mean to catch). Task-forming
macros are excluded from the rewrite, matching the existing exclusion of
closures. break/continue — which bypass both the commit and any catch — now
commit via a finally, making every non-exceptional exit consistent: only a
thrown exception rolls back. Documented in the docstring.

Regression tests cover nested return, both catch shapes, @Spawn, break,
continue, and recursive re-entry of the same expansion; removing the fix
fails six of them plus downstream testsets poisoned by the stuck-open
transaction.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread src/Postgres.jl Outdated
h(x) = ... parses as :(=) with a call-shaped left-hand side, not as
:function, so the rewrite's closure exclusion missed it: a return inside a
local short-form helper defined in the @transaction body was rewritten into
a transaction-return marker. Calling such a helper silently early-returned
the ENCLOSING function with the helper's internal value (committing on the
way out), and a helper that escaped the block threw a raw TransactionReturn
at its caller with no expansion active to catch it.

All short-form shapes are skipped (plain, ::T return-type, where-clauses,
qualified names), while ordinary assignments whose right-hand side contains
a return are still rewritten. Also adds @spawnat to the task-macro skip
list — same bug class as @spawn/@async, verified to wrap the marker in a
RemoteException instead of producing the task's value.

Live test: a short-form helper with an internal early return, used inside
the block and after it escapes. Unit pins for every definition shape, the
task macros, and the ordinary-assignment counter-cases. Removing the skip
fails six of them.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread src/Postgres.jl Outdated
`return` anywhere inside a comprehension or generator — body or iterator
expression — is a lowering error in plain Julia. The rewrite turned it
into a legal `throw` of the transaction-return marker, silently accepting
code that would stop compiling the moment the @transaction wrapper is
removed, and giving it early-return semantics it never legitimately had.
Comprehension, typed-comprehension, generator, and flatten heads are now
left untouched so the construct errors exactly as it does everywhere else.

Nothing valid is lost: a legal comprehension cannot contain a bare
`return`, and nested closures inside one were already excluded. Unit pins
cover all four syntactic shapes plus the counter-case that a `return`
inside an ordinary `for` loop is still rewritten.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread src/Postgres.jl Outdated
quinnj and others added 2 commits August 7, 2026 13:44
Distributed.@fetch and @fetchfrom wrap their body in a remotely-executed
thunk whose return is the fetched value, exactly like @spawnat — but they
were missing from _TASK_MACROS, so a return inside one was rewritten into
a transaction-return marker. Verified live: the block then throws a
RemoteException wrapping the marker and rolls back, where plain Julia
returns the value.

Also corrects the comprehension-skip rationale in comments: a return in a
comprehension/generator BODY is a lowering error (which the rewrite must
not legalize), while the iterator-expression shapes lowering does accept
behave correctly un-rewritten — they exit the block non-exceptionally and
commit through the expansion's finally, as verified live. And documents at
the token comparison that unconditional returning would be observationally
equivalent today only because every enclosing expansion's finally also
commits; the token check stays as the semantic guarantee.

Independent adversarial verification of the three @transaction commits
(61 live scenarios, plain-Julia baselines, 6 mutations against the full
suite) found no other behavioral gaps.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nally

Review of the rewrite approach (PR #7 threads) proved its task-macro
allowlist structurally insufficient: any third-party macro that wraps its
body in a task or closure — reproduced with a minimal @local_task — had its
internal returns rewritten into transaction-return markers, throwing
TaskFailedException(TransactionReturn) instead of producing the task's
value. No finite list of standard macros covers user-defined ones.

The expansion's finally already gives plain `return` the intended semantics
with no rewriting at all: a return unwinds through every enclosing
expansion's finally, each committing its level exactly once, innermost
first. User catches cannot intercept a plain return, closures and task
macros keep their ordinary meaning untouched, and the flattened-iterator
form that plain lowering accepts behaves identically wrapped or not. The
marker struct, the AST walker, the try-guard injection, and both skip lists
are deleted.

The finally also now handles its own commit failure: it rolls back the
current level before propagating (commit at savepoint depth leaves depth
unchanged on failure), so every enclosing level — macro expansion or plain
catch — unwinds its own. Previously a RELEASE SAVEPOINT failure during a
break out of a nested level (savepoint aborted by a swallowed server error)
escaped past the enclosing macro's ability to clean up, leaving the outer
transaction open with its work pending.

Behavioral regressions replace the deleted unit AST pins: a third-party
@local_task macro, Distributed @spawnat/@fetch/@fetchfrom run locally on
worker 1, plain-vs-wrapped flattened-iterator equivalence, and the nested
break with an aborted savepoint (asserts the server error surfaces and
nothing stays open client- or server-side; removing the finally rollback
fails five assertions).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@quinnj

quinnj commented Aug 7, 2026

Copy link
Copy Markdown
Member Author

READY/CLEAN on exact head 467596923c86b4f483d107174b8a28d4edd7f476.

Independent PostgreSQL 16 retests passed: standard and third-party task macros preserve plain return semantics and commit; a failed finally savepoint commit now unwinds every client/server transaction level and rolls back pending rows; the flattened-iterator case matches plain Julia and commits. All 15 exact-head CI jobs and both Codecov checks are green. The PR is MERGEABLE/CLEAN with no unresolved review threads.

@quinnj
quinnj merged commit 77b9296 into main Aug 7, 2026
17 checks passed
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.

1 participant