WiP: first test to try to resolve #507

[FCO]

If red-do :transaction's block returns a Awaitable, it creates
a new Promise that will handle the commit/rollback and does not
let the sync code to call commit/rollback.

Probable next steps are:

• extract that logic to a transaction manager
• on transaction manager add a transaction stack and methods
  • queue-begin
  • unqueue-commit
  • break-queue-rollback
    (or something like that)

[hermes-fco]

Code Review — PR #508 (WiP: first test to try to resolve #507)

PR #508 addresses issue #507: async/awaitable transaction handling in red-do :transaction. When the transaction block returns a Promise, the synchronous KEEP/UNDO phasers fire immediately (before resolution), causing premature commit/rollback.

🔴 Critical

None — this is a WiP from 2021 with no obvious security or correctness regressions in the diff.

⚠️ Warnings

• lib/Red/Do.pm6 transaction dispatch — The new Promise-aware dispatch correctly detects Promise returns and wraps them with KEEP/UNDO inside a start block. However, the logic for when !*.DEFINITE (nil return) doing UNDO $conn.rollback may be too aggressive — an explicit return Nil from a transaction block might be intentional. Consider only rolling back on Failure or Exception.

• lib/Red.pm6 experimental role system — The change from directly adding roles (^add_role) to queueing them in @experimental-roles and applying via new method override is a non-trivial behavioral change. Roles are now applied at instantiation time rather than compose time, which could break precompilation caching or change initialization order. The intent appears to be lazy/deferred role application (likely for performance), but the semantics differ from the original approach.

💡 Suggestions

• lib/Red/Do.pm6 — red-do :transaction refactoring: The new logic is a strict improvement over the previous KEEP/UNDO-on-caller approach. The pattern of wrapping Promise returns is correct. Consider also handling Supply returns (if transactions can produce streaming results).

• DirtableWrapped role (same pattern as in PR WiP: .^create from belongs-to relationship #523) — Guards against double-wrapping TWEAK when models are composed multiple times. Good defensive pattern that should be ported independently.

• Typo fixes: column-formater → column-formatter, Red::Formater → Red::Formatter, experimental("formaters") → experimental("formatters"). These are cosmetic but improve consistency.

🔍 Minor observations

• lib/MetamodelX/Red/Dirtable.pm6 — Same set-helper-attrs refactoring as PR WiP: .^create from belongs-to relationship #523 (using .map + Hash instead of is Set). This pattern has likely been addressed in main through independent PRs.
• lib/MetamodelX/Red/Id.pm6 — Same id-hash addition as PR WiP: .^create from belongs-to relationship #523.
• lib/MetamodelX/Red/Model.pm6:new — Experimental role injection at .new time. This @experimental-roles + ^mixin approach is a significant architectural change.
• lib/Red.pm6:Red.ping — New .ping method that delegates to driver ping. Simple, useful addition.

✅ Looks Good

• The core async transaction fix (Promise detection + deferred KEEP/UNDO) solves the described problem from Concurrency *within* transactions. #507.
• The CATCH block in red-do :transaction properly re-throws after rolling back on exceptions.

Verdict

This is a WiP PR from August 2021. The core async transaction fix is sound. Much of the surrounding refactoring (typo fixes, experimental role system, helper attributes) has been incorporated into main through subsequent development. Consider extracting just the Red::Do.pm6 transaction fix into a focused PR — that's the valuable contribution here.

---

Reviewed by Hermes Agent