fix(policy): always wrap mutation in a transaction

[ymc9]

Summary by CodeRabbit

• New Features

  • Mutations now run in an automatic managed transaction when not already in one, improving consistency and rollback behavior.

• Refactor

  • Streamlined hook execution and unified connection propagation for more predictable query and hook behavior.
  • Error types consolidated under a common base for more consistent error handling.

• Tests

  • Updated tests to use the current query builder API and result fields.

[coderabbitai]

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Reworks the query executor to pass explicit connections, manage mutations within optional transactions, and streamline hook/interceptor flows; introduces a ZenStackError base class and updates several error types; and adjusts policy tests and migrated test setup to current APIs and imports.

Changes

Cohort / File(s)	Summary of Changes
Runtime executor refactor packages/runtime/src/client/executor/zenstack-query-executor.ts	Adds ZenStackError handling; wraps executeQuery in provideConnection; begins/commits/rolls back transactions for mutations when needed; passes explicit connection through proceedQueryWithKyselyInterceptors and proceedQuery; streamlines mutation hook flow (before/after sequencing, post-commit callbacks) and removes previous accessor for in-transaction after-mutation checks.
Errors hierarchy update packages/runtime/src/client/errors.ts	Introduces ZenStackError base class and changes InputValidationError, QueryError, InternalError, and NotFoundError to extend it (inheritance-only change).
Policy plugin error change packages/runtime/src/plugins/policy/errors.ts	RejectedByPolicyError now extends ZenStackError and accepts an optional message?: string, defaulting to a constructed message; adds import from ../../client.
Policy test — CRUD update packages/runtime/test/policy/crud/post-update.test.ts	Unskips and updates test to use executeTakeFirst() instead of execute() and assert numUpdatedRows (renamed from numAffectedRows); adjusts expected updated values and row-count assertions.
Migrated policy test cleanup packages/runtime/test/policy/migrated/multi-field-unique.test.ts	Removes legacy test setup boilerplate and lifecycle imports (path, beforeAll, afterEach, origDir logic); reorders imports and retains runtime test logic with simplified vitest imports.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• refactor: remove filter from entity mutation hooks #208 — Similar changes to mutation hook/interceptor flow and threading of connection/query context in the executor.
• fix issues with nested read; getting more test cases to pass #6 — Overlaps in executor error handling and proceedQuery/executeQuery control flow changes.
• feat: implement raw queries #69 — Related signature and context propagation changes for proceedQuery/proceedQueryWithKyselyInterceptors.

Poem

I hop through code with whiskers twitching bright,
Transactions snug as burrows through the night.
Hooks nibble before, then clap after too,
Connections in paw, I tidy the queue.
Tests hum like crickets — updated rows in sight 🥕

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title concisely and accurately describes the main change of ensuring all mutations are wrapped in a transaction, which directly reflects the core modifications made to the executor and related flows.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.

✨ Finishing touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/post-update-tx

---

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9cb3514 and 519b557.

📒 Files selected for processing (4)

• packages/runtime/src/client/errors.ts (3 hunks)
• packages/runtime/src/client/executor/zenstack-query-executor.ts (4 hunks)
• packages/runtime/src/plugins/policy/errors.ts (2 hunks)
• packages/runtime/test/policy/migrated/multi-field-unique.test.ts (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

{packages,samples,tests}/**

📄 CodeRabbit inference engine (CLAUDE.md)

Place packages only under packages/, samples/, or tests/

Files:

• packages/runtime/src/client/executor/zenstack-query-executor.ts
• packages/runtime/src/plugins/policy/errors.ts
• packages/runtime/src/client/errors.ts
• packages/runtime/test/policy/migrated/multi-field-unique.test.ts

🧬 Code graph analysis (2)

packages/runtime/src/client/executor/zenstack-query-executor.ts (2)

packages/runtime/src/client/executor/zenstack-driver.ts (2)

• compiledQuery (181-188)
• connection (129-170)

packages/runtime/src/client/errors.ts (2)

• ZenStackError (4-4)
• QueryError (18-22)

packages/runtime/src/plugins/policy/errors.ts (1)

packages/runtime/src/client/errors.ts (1)

• ZenStackError (4-4)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: build-test (20.x, postgresql)
• GitHub Check: build-test (20.x, sqlite)

🔇 Additional comments (16)

packages/runtime/test/policy/migrated/multi-field-unique.test.ts (1)

1-3: LGTM! Clean removal of legacy test boilerplate.

The import cleanup correctly removes unused test lifecycle hooks and directory management while retaining the necessary test utilities. The tests continue to use QueryError which now properly extends ZenStackError as per the broader error hierarchy changes in this PR.

packages/runtime/src/plugins/policy/errors.ts (2)

1-2: LGTM! Proper integration with new error hierarchy.

The import of ZenStackError correctly sets up the base class for the policy error types.

---

26-33: LGTM! Error hierarchy updated correctly.

RejectedByPolicyError now properly extends ZenStackError with a well-structured constructor that provides a clear default message while allowing custom messages. The change maintains backward compatibility since the message parameter remains optional.

packages/runtime/src/client/errors.ts (2)

1-4: LGTM! Clean base error class introduction.

The ZenStackError base class provides a unified foundation for all ZenStack runtime errors, enabling consistent error handling and type checking across the codebase.

---

9-36: LGTM! Error hierarchy properly updated.

All error classes now correctly extend ZenStackError while maintaining their existing constructor signatures and behavior. This ensures backward compatibility while establishing a consistent error hierarchy.

packages/runtime/src/client/executor/zenstack-query-executor.ts (11)

73-106: Transaction wrapping logic looks solid, but verify isolation level choice.

The transaction management correctly wraps mutations in a transaction when not already in one, with proper commit/rollback handling. Error wrapping preserves ZenStackError instances while wrapping other errors in QueryError with SQL context.

However, the isolation level change from ReadCommitted to RepeatableRead (line 79) is a significant behavioral change that affects concurrent access patterns and may impact performance.

Please document the rationale for choosing RepeatableRead isolation level. Consider adding an inline comment explaining why this level is necessary for mutation transactions, or confirm whether this should be configurable.

Note: This echoes an earlier review comment about the isolation level change.

---

108-141: LGTM! Connection propagation through interceptors is correct.

The explicit connection parameter is properly threaded through the Kysely interceptor chain via closure, ensuring all hooks execute with the same connection context. The simplified return of hookResult (line 134) removes unnecessary wrapping.

---

171-179: LGTM! Efficient fast path for queries without mutation hooks.

The fast path correctly skips mutation hook machinery when unnecessary, directly executing the query with proper name mapping and parameter handling.

---

181-191: LGTM! Ensures complete entity data for after-mutation hooks.

Properly adds returnAll to insert/update queries when after-mutation hooks need access to the complete mutated entities. This is necessary because hooks may need to inspect all fields, not just the ones explicitly selected.

---

198-222: LGTM! Before-mutation hook setup is well-structured.

The code correctly:

• Creates a connection-specific client that respects the current transaction state
• Prepares mutation info extraction
• Implements lazy loading of before-mutation entities with caching
• Invokes before-mutation hooks with proper context

---

224-237: LGTM! After-mutation hook timing is correctly managed.

The conditional logic properly handles two scenarios:

• Outside transaction: All after-mutation hooks run immediately after the query
• Inside transaction: Hooks marked for in-transaction execution run immediately; others are registered as post-commit callbacks

This ensures hooks requesting to run within the transaction see uncommitted state, while others see committed state, giving plugins fine-grained control over their execution timing.

---

242-250: LGTM! Client creation for connection context is correct.

The helper properly creates a client bound to the specific connection with mutation hooks suppressed (to avoid infinite recursion) and transaction mode forced when already in a transaction.

---

350-374: LGTM! Before-mutation hook invocation is straightforward.

The method correctly iterates through plugins and invokes beforeEntityMutation hooks with the necessary context, including the lazy entity loader.

---

376-428: LGTM! After-mutation hook filtering and invocation is correct.

The filtering logic (lines 393-399) properly segregates hooks based on their runAfterMutationWithinTransaction preference, ensuring:

• filterFor: 'inTx' → only hooks requesting in-transaction execution
• filterFor: 'outTx' → only hooks NOT requesting in-transaction execution
• filterFor: 'all' → all hooks (used when not in a transaction)

The loadAfterMutationEntities callback correctly returns mutated rows for non-delete operations.

---

430-446: LGTM! Entity loading for before-mutation state is correct.

The method builds a select query filtered by the mutation's where clause and executes it directly on the provided connection, properly loading the state before the mutation for comparison in hooks.

---

68-68: Critical: Method signature missing async keyword.

The executeQuery override is missing the async keyword but contains await operations and returns a Promise via provideConnection. This will cause a compilation error.

Apply this diff:

-    override executeQuery(compiledQuery: CompiledQuery, queryId: QueryId) {
+    override async executeQuery(compiledQuery: CompiledQuery, queryId: QueryId) {

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🧪 Early access (Sonnet 4.5): enabled

We are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience.

Note:

• Public repositories are always opted into early access features.
• You can enable or disable early access features from the CodeRabbit UI or by updating the CodeRabbit configuration file.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Claude finished @ymc9's task —— View job

---

PR Review: Transaction Wrapping for Mutations

Todo List:

• Read and analyze the modified files
• Review transaction implementation changes
• Analyze test modifications
• Provide comprehensive code review feedback
• Check for potential bugs, performance, and security issues

Review Summary

This PR implements a significant architectural improvement by always wrapping mutations in transactions. The changes simplify the transaction handling logic while ensuring data consistency for all mutation operations.

Key Changes

1. Simplified Transaction Management (zenstack-query-executor.ts:68-100)

• Improvement: Moved transaction creation to the top level of executeQuery()
• Benefit: All mutations now automatically get transaction protection
• Logic: Only creates a new transaction if not already in one (!this.driver.isTransactionConnection(connection))
• Isolation Level: Uses RepeatableRead for consistency

2. Removed Complex Conditional Transaction Logic

• Simplification: Eliminated the hasPluginRequestingAfterMutationWithinTransaction logic
• Removed: ~70 lines of complex conditional transaction creation code
• Result: Much cleaner, more predictable behavior

3. Test Coverage Improvement (post-update.test.ts)

• Fixed: Re-enabled previously skipped test for query builder API
• Updated: Corrected test expectations for Kysely API changes (numAffectedRows → numUpdatedRows)

Code Quality Assessment

✅ Strengths

1. Architectural Improvement: The "always wrap in transaction" approach is much simpler and safer
2. Error Handling: Proper try-catch with rollback on errors
3. Performance: Avoids nested transactions by checking isTransactionConnection()
4. Consistency: All mutations now have the same transaction behavior
5. Code Simplification: Removed complex conditional logic (-154 lines, +101 lines net)

⚠️ Potential Concerns

1. Performance Impact: Every mutation now requires a transaction, even simple ones

   • Mitigation: The cost is likely minimal for most use cases
   • Benefit: Consistency and reliability outweigh the small performance cost

2. Transaction Isolation Level: Uses RepeatableRead instead of the previous ReadCommitted

   • Impact: Stronger consistency but potentially more contention
   • Recommendation: Document this change and monitor for deadlock issues

3. Connection Pool Usage: Transactions hold connections longer

   • Impact: Could affect connection pool utilization under high load
   • Monitoring: Watch for connection pool exhaustion

Security & Best Practices

✅ Security

• Data Integrity: Transactions ensure atomic operations
• Consistency: Prevents partial updates that could leave data in inconsistent state
• Rollback Protection: All mutations are properly rolled back on errors

✅ Best Practices

• Separation of Concerns: Transaction logic is cleanly separated
• Error Handling: Comprehensive error handling with proper cleanup
• Code Readability: Much more readable than the previous complex logic

Performance Considerations

1. Transaction Overhead: Small but consistent overhead for all mutations
2. Lock Duration: RepeatableRead may hold locks longer than ReadCommitted
3. Connection Usage: Transactions require dedicated connections

Recommendation: Monitor database performance metrics after deployment, especially:

• Transaction deadlocks
• Connection pool utilization
• Query execution times

Test Coverage

The test changes are appropriate:

• Re-enabled a previously skipped test, indicating the transaction issue was resolved
• Updated API calls to match current Kysely expectations
• Maintains comprehensive test coverage for post-update policies

Overall Assessment

This is a high-quality improvement that trades a small performance cost for significant gains in:

• Simplicity: Much cleaner, easier-to-understand code
• Reliability: All mutations are now consistently protected by transactions
• Maintainability: Reduced complexity makes future changes easier

The architectural decision to always wrap mutations in transactions is sound and aligns with database best practices. The code quality is excellent with proper error handling and clean separation of concerns.

Recommendation: ✅ Approve - This is a positive change that improves the codebase architecture and reliability.

[Copilot]

Pull Request Overview

This PR fixes a transaction handling issue in the policy system by ensuring all mutations are consistently wrapped in transactions, simplifying the complex conditional transaction logic.

• Removes the conditional transaction creation based on plugin requirements and instead wraps all mutations in transactions by default
• Simplifies the query execution flow by eliminating the complex nested transaction handling logic
• Updates test expectations to use the correct API methods (executeTakeFirst instead of execute and numUpdatedRows instead of numAffectedRows)

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
packages/runtime/src/client/executor/zenstack-query-executor.ts	Refactored transaction handling to always wrap mutations in transactions and simplified the query execution flow
packages/runtime/test/policy/crud/post-update.test.ts	Re-enabled a skipped test and updated API calls to use correct methods and property names

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[claude]

Claude finished @ymc9's task —— View job

---

PR Review: Transaction Wrapping for Mutations

Todo List:

• Read and analyze the modified files
• Review transaction implementation changes
• Analyze test modifications
• Provide comprehensive code review feedback
• Check for potential bugs, performance, and security issues

Review Summary

This PR implements a significant architectural improvement by always wrapping mutations in transactions. The changes simplify the transaction handling logic while ensuring data consistency for all mutation operations.

Key Changes Analysis

1. Simplified Transaction Management (zenstack-query-executor.ts:77-82)

• Improvement: Moved transaction creation to the top level of executeQuery()
• Benefit: All mutations now automatically get transaction protection
• Logic: Only creates a new transaction if not already in one (!this.driver.isTransactionConnection(connection))
• Isolation Level: Uses RepeatableRead for consistency

2. Removed Complex Conditional Transaction Logic

• Simplification: Eliminated complex conditional transaction creation based on plugin requirements
• Removed: Approximately 70 lines of complex conditional transaction logic
• Result: Much cleaner, more predictable behavior

3. Test Coverage Updates (post-update.test.ts:156-165)

• Fixed: Updated test to use current Kysely API (executeTakeFirst() instead of execute())
• Corrected: Changed assertion from numAffectedRows to numUpdatedRows
• Validation: Tests now properly validate the transaction behavior

Code Quality Assessment

✅ Strengths

1. Architectural Improvement: The "always wrap in transaction" approach is much simpler and safer
2. Error Handling: Proper try-catch with rollback on errors (zenstack-query-executor.ts:93-96)
3. Performance: Avoids nested transactions by checking isTransactionConnection()
4. Consistency: All mutations now have the same transaction behavior
5. Code Simplification: Significant reduction in complex conditional logic

⚠️ Potential Concerns

1. Transaction Isolation Level Change:

   • Change: Uses RepeatableRead instead of previous default
   • Impact: Stronger consistency but potentially more lock contention
   • Recommendation: Monitor for deadlock issues in production

2. Performance Impact:

   • Cost: Every mutation now requires a transaction, even simple ones
   • Mitigation: The overhead is likely minimal for most use cases
   • Benefit: Consistency and reliability outweigh the small performance cost

3. Connection Pool Usage:

   • Impact: Transactions hold connections longer than before
   • Monitoring: Watch for connection pool exhaustion under high load

Security & Best Practices

✅ Security

• Data Integrity: Transactions ensure atomic operations
• Consistency: Prevents partial updates that could leave data in inconsistent state
• Rollback Protection: All mutations are properly rolled back on errors

✅ Best Practices

• Separation of Concerns: Transaction logic is cleanly separated
• Error Handling: Comprehensive error handling with proper cleanup
• Code Readability: Much more readable than the previous complex logic

Specific Code Issues

🔧 Method Signature Issue (zenstack-query-executor.ts:68)

The copilot reviewer correctly identified that the method signature appears to have changed from async to sync, but the method body still contains async operations. However, looking at the code, the method is still properly async and returns a Promise through the provideConnection call.

📝 Documentation Opportunity (zenstack-query-executor.ts:77-82)

The transaction isolation level change from default to RepeatableRead is significant and could benefit from a comment explaining the reasoning.

Test Coverage Analysis

The test changes are appropriate and demonstrate good practices:

• API Updates: Properly updated to use current Kysely API methods
• Assertion Corrections: Fixed property names to match current API
• Coverage: Maintains comprehensive test coverage for post-update policies
• Validation: Tests verify both successful updates and proper rejection behavior

Performance Considerations

1. Transaction Overhead: Small but consistent overhead for all mutations
2. Lock Duration: RepeatableRead may hold locks longer than previous isolation levels
3. Connection Usage: Transactions require dedicated connections

Recommendation: Monitor database performance metrics after deployment, especially:

• Transaction deadlocks
• Connection pool utilization
• Query execution times

Overall Assessment

This is a high-quality improvement that trades a small performance cost for significant gains in:

• Simplicity: Much cleaner, easier-to-understand code
• Reliability: All mutations are now consistently protected by transactions
• Maintainability: Reduced complexity makes future changes easier

The architectural decision to always wrap mutations in transactions is sound and aligns with database best practices. The code quality is excellent with proper error handling and clean separation of concerns.

Recommendation: ✅ Approve - This is a positive change that improves the codebase architecture and reliability.

---