fix(#9970): an interrupted nested begin() pops only the level it pushed

[robfrank]

Closes #9970

Summary

Database.run_in_transaction and TransactionContext.__enter__ (with db.transaction():) decided what to roll back from a boolean taken before begin() (was_active) and, in run_in_transaction, a begun flag set after it. Inside a caller's own transaction LocalDatabase.begin() does not refuse, it pushes a NESTED level. So an interrupt landing after the push but before begin() returned left begun=False, was_active=True, nothing popped the new level, and the caller's next commit() committed that empty level and popped it, leaving the caller's own transaction open and their work uncommitted (silent data loss rather than a leaked handle).

Ownership is now decided by the transaction stack as it was before begin(), as the issue suggests: a new Database._transaction_stack_mark() captures (is_transaction_active(), getNestedTransactions()), and Database._end_transactions_opened_since(mark) rolls back every level above the marked depth, then the transaction that became active where none was active. One helper, two callers, no begun flag.

The second clause is needed because the depth alone does not see the non-nested case: begin() with no active transaction reuses the base context (depth stays 1 when an inactive context already exists), so "depth grew" and "a transaction became active" are both ownership signals.

Finding ledger

#	Finding (from the issue)	Status
1	run_in_transaction leaves an interrupted nested level on the stack (core.py:478)	fixed here
2	TransactionContext.__enter__ has the same gap (transactions.py:27), and __exit__ never runs	fixed here
3	insert_many / graph-batch paths cannot nest (guarded by if not was_active)	argued, verified below

Completeness

Invariant: a transaction-opening helper in the Python binding rolls back exactly the transaction levels it opened, never a level the caller held when it was called.

grep -rn "\.begin()" bindings/python/src:

Entry point	Can nest?	Status
core.py run_in_transaction	yes, begin() unconditional	fixed here, tests test_interrupt_after_a_nested_begin_pops_only_that_level, test_a_failing_block_inside_a_callers_transaction_rolls_back_its_level, existing #9813 tests
transactions.py TransactionContext.__enter__	yes, begin() unconditional	fixed here, new TestTransactionContextBeginHygiene (3 tests)
core.py insert_many (begin at 677 / 694)	no: if not was_active	argued: only begins when nothing is active, so begin() never pushes
core.py per-row/graph path at 762	no: if not was_active	argued, same guard
graph_batch.py create_vertex	no: if not isTransactionActive()	argued, same guard
DocumentBatcher.java insertManyJson / insertColumns	no: if (!wasActive)	argued, same guard

Behavior note: in run_in_transaction with a caller transaction held, a block that fails AFTER its nested level was already committed/popped (e.g. a failed nested commit, which pops in LocalDatabase.commit()'s finally) used to roll back the caller's level too (begun was True). The depth comparison leaves the caller's level alone, which is the ownership rule this fix establishes. A block that leaves extra levels of its own open is now unwound completely instead of by one level.

Test plan

• New test_interrupt_after_a_nested_begin_pops_only_that_level (run_in_transaction and with db.transaction():): caller holds a transaction with one saved row, begin() is patched to run the real begin then raise KeyboardInterrupt; asserts depth is back to 1, the caller's transaction is still active, and the caller's commit persists the row. Both FAILED before the fix (assert 2 == 1) and pass after.
• New TransactionContext tests for the non-nested interrupt and the refused begin() inside a caller's transaction.
• New test_a_failing_block_inside_a_callers_transaction_rolls_back_its_level.
• tests/test_interrupt_hygiene.py tests/test_run_in_transaction_retry.py tests/test_sigint.py tests/test_transaction_config.py tests/test_core.py tests/test_cross_model_atomicity.py tests/test_concurrency.py: all pass except test_to_columns_typed_bulk_materialization (numpy not installed locally).
• test_bulk_insert, test_graph_batch, test_bridge_fixes, test_async_executor, test_graph_api, test_columnar_readers, test_insert_columns: identical failure set (24, locally stale arcadedb-python-bridge.jar without DocumentBatcher, plus numpy) with and without the patch, diffed.
• Local run used the current arcadedb-engine-26.11.1-SNAPSHOT.jar; CI's Python job runs against freshly built jars.

Coverage

Covered: Database.run_in_transaction, TransactionContext.__enter__ (with db.transaction():).

Known gaps: None. The remaining begin() callers only begin when no transaction is active and cannot push a nested level.

Adversarial pass

The Phase 1.5 independent subagent could not be spawned in this run (no subagent tool available to this agent); a self-review was done instead against the issue body and the diff:

• Fallback when getNestedTransactions() is unavailable: not real, it is declared on BasicDatabase, so every Java database the binding wraps has it.
• Loop termination: the rollback loop is bounded by the depth measured once, and stops early when nothing is active.
• Interrupt during the mark itself: the mark is taken before the try, before anything is opened, same as the former was_active read.

Review cycles

Cycle	Head	Changes	Bot outcome
1	5a371ef	initial fix + tests	claude: LGTM, 6 non-blocking nits
2	c17525f	added test_a_failed_nested_commit_leaves_the_callers_transaction_open (fails on main: the caller's level was rolled back), TransactionContext failing-block test for __exit__, try/except around fixture teardown	claude: LGTM, suggested a failing rollback should not skip lower levels
3	b43126a	rollback loop continues past a failing rollback and re-raises the first error; test_a_failing_rollback_does_not_skip_the_levels_below_it (fails on cycle-2 code)	claude: LGTM, 4 nits
4	ede726f	final rollback cannot mask an earlier error; comment on why the mark stays above begin()	claude review run stayed queued past the 15 min window (runner backlog), no review
-	2cc669a	Bandit B110 on the fixture's best-effort teardown (introduced in cycle 2): # nosec B110, as the rest of the binding does	not reviewed (max cycles reached)

Deferred / skipped review items

• Cycle 1 Bump gremlin.version from 3.4.10 to 3.5.1 #2 "use a public nesting-depth accessor if Database has one": none exists; getNestedTransactions() is declared on BasicDatabase, so the private helper calls it directly. Skipped.
• Cycle 1 Bump jedis from 3.6.1 to 3.6.3 #5 (final keyword) and Bump metrics.version from 4.2.2 to 4.2.3 #6 (placeholder section): not applicable / done here.
• Cycle 2 build support #1 "changelog line for the behaviour change": left to the release notes, the change is documented above.
• Cycle 2 Bump gremlin.version from 3.4.10 to 3.5.1 #2 / cycle 3 build support #1 "JNI cost of the mark": skipped, one getNestedTransactions() call next to begin(); is_transaction_active() cannot be derived from the depth because an inactive base context still counts as one level.
• Cycle 2 Bump slf4j-simple.version from 1.7.31 to 1.7.32 #3 "private helpers used from transactions.py": same package, and TransactionContext only wraps Database. Skipped.
• Cycle 2 Bump mongo-java-driver from 3.12.4 to 3.12.10 #4 "rollbackAllNested() failure is silent in the fixture": intended, it only keeps drop() working after a failed assertion; the assertions themselves check the depth.
• Cycle 3 Bump mongo-java-driver from 3.12.4 to 3.12.10 #4 "monkeypatched begin is not a real async KeyboardInterrupt": accepted; real-signal delivery is covered by test_sigint.py, the window here cannot be hit deterministically with a real signal.

Final state

max-cycles-reached (4 cycles; the cycle-4 review did not arrive within the window). Every review so far was LGTM with non-blocking nits.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Improved transaction cleanup when an operation is interrupted or fails, including during nested transaction setup and commits.
  • Cleanup now preserves transactions that were already active, so callers can continue and commit their work.
  • Rollback attempts continue across newly opened transaction levels, even if one rollback fails.

[mergify]

This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again.

[codacy-production]

Not up to standards ⛔

🔴 Issues 2 minor

Alerts:
⚠ 2 issues (≤ 0 issues of at least minor severity)

Results:
2 new issues

Category	Results
Documentation	2 minor

View in Codacy

🟢 Metrics 9 complexity

Metric	Results
Complexity	9

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 1ec35e9a-d19c-4941-b3b9-faaba8474070

📥 Commits

Reviewing files that changed from the base of the PR and between b43126a and d7a3e24.

📒 Files selected for processing: 3

• bindings/python/src/arcadedb_embedded/core.py
• bindings/python/tests/test_interrupt_hygiene.py
• bindings/python/tests/test_transaction_context_interrupt.py

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

📝 Walkthrough

run_in_transaction and TransactionContext now record transaction activity and nesting depth before begin(). On failure, they clean up transaction levels opened after that mark. Tests cover interrupts, refused begins, nested transactions, and rollback errors.

Changes: Transaction interruption cleanup

Layer / File(s)	Summary
Stack marking, cleanup, and regression coverage bindings/python/src/arcadedb_embedded/core.py, bindings/python/src/arcadedb_embedded/transactions.py, bindings/python/tests/test_interrupt_hygiene.py, bindings/python/tests/test_transaction_context_interrupt.py	Both transaction APIs clean up levels opened after a saved stack mark. Cleanup continues after rollback errors and raises the first rollback error. Tests check nested transaction state, caller-owned work, and transaction depth after failures.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: lvca

Merge Risk: ⚪ Minimal · up to d7a3e

No issue requiring a fix before merge was established; normal checks remain appropriate.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 13.51% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 37 functions across 4 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title clearly identifies the fix for interrupted nested begin() calls and accurately summarizes the main change.
Description check		The description is detailed and on topic. It explains the motivation, related issue, implementation, test coverage, known gaps, and review status. It does not use the exact template headings and does …
Linked Issues check		Issue #9970 requires cleanup in Database.run_in_transaction and TransactionContext.__enter__ after an interrupted nested begin(). The PR records the pre-begin() activity and stack depth, then …
Out of Scope Changes check		The production changes stay within issue #9970. The shared helper supports the two affected unconditional begin() paths. The added tests and fixture cleanup validate transaction-stack hygiene. No un…

• Fix all pre-merge checks with AI

✨ Finishing Touches: 💡 1

📝 Generate docstrings: 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

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

[claude]

Review

I read the diff and checked it against LocalDatabase.getNestedTransactions() and rollback(). I did not run the tests.

Summary: The fix is sound. Deciding ownership from the stack as it was before begin() closes the #9970 window, and it also keeps the #9813 behaviour where a refused begin() leaves the caller's level alone.

Checked

• getNestedTransactions() returns transactions.size(), or 0 when no context exists. rollback() goes through popIfNotLastTransaction(), so the last level is never popped.
• Non-nested, no context: the mark is (False, 0) and after begin() the depth is 1. The loop rolls back once, then the second clause sees nothing active. Correct.
• Non-nested, inactive base context already present: the mark is (False, 1) and the depth stays 1. The loop does zero iterations and the not was_active clause does the rollback. This is the case the PR description calls out, and the logic handles it.
• Nested: the mark is (True, n). The loop pops only the levels above n, and the caller's level is untouched.

Suggestions (non-blocking)

1. Behaviour change in run_in_transaction. The PR already documents that a block failing after its own nested level was already popped no longer rolls back the caller's level. That is the right rule. It would still help to add one regression test for the failed nested commit case, to pin it. Today only the plain ValueError case is covered.
2. Wrapper lookups. _transaction_stack_mark calls self._java_db.getNestedTransactions() directly, while the rest of the class goes through wrapper methods. If Database exposes a public nesting-depth accessor, use it. Otherwise this is fine. The per-call JNI cost is negligible next to begin().
3. Test fixture. rollbackAllNested() in the fixture teardown is a good safety net. Consider guarding it with try/except, so a teardown failure cannot hide the original assertion failure.
4. Test gap. TransactionContext.__exit__ is untouched here. It is worth confirming that the normal exit path still behaves correctly when the context is entered inside a caller's transaction and the block raises. The new tests only cover __enter__.
5. Convention. CLAUDE.md asks for final where possible, but that does not apply to Python. The style matches the surrounding code.
6. PR description. The "Review cycles" section is still a placeholder. Fill it in or remove it.

Test coverage is good: the interrupt, refused-begin and failing-block scenarios are all covered for both entry points.

Overall: LGTM, with the nits above.

[claude]

Review

Overall this is a solid fix. Deciding ownership from the transaction stack captured before begin(), instead of a begun flag set after it, closes the window cleanly. One shared helper for both callers is also the right shape.

What I like

• _transaction_stack_mark() and _end_transactions_opened_since() replace two ad-hoc checks, so run_in_transaction and TransactionContext.__enter__ can no longer drift apart.
• The loop is bounded by the depth measured once, and it breaks early if nothing is active, so a rollback that pops nothing cannot spin.
• The tests mirror the real failure. They patch begin() to run the real one and then raise, and they assert on depth as well as on persisted data.
• The teardown rollbackAllNested() keeps one failed assertion from cascading into drop() errors.

Points to consider (non-blocking)

1. Behaviour change. A failed nested commit used to roll back the caller's level too, because begun was True. It now leaves the caller's transaction open. The PR description and test_a_failed_nested_commit_leaves_the_callers_transaction_open cover this and I agree with the new rule. A line in the changelog or docs might help anyone who relied on the old behaviour.
2. Extra JNI calls. getNestedTransactions() adds one JNI call per attempt, and one more on the error path. That is negligible, but the mark could be taken lazily if run_in_transaction shows up in hot-path profiles.
3. Private method used across modules. transactions.py calls database._transaction_stack_mark() and _end_transactions_opened_since(). This is fine inside one package, but if TransactionContext can wrap other database objects, a short note on the contract would help.
4. Assumption in the test fixture. rollbackAllNested() needs to exist on the Java database type the fixture wraps. It is wrapped in try/except, so a missing method is silent. That is acceptable, but it could hide a leak in the tests.
5. Rollback errors. rollback() failures inside the helper are swallowed by the callers' except Exception. If the first rollback in the loop throws, the remaining levels are skipped. Continuing the loop in a per-iteration try/except would make the cleanup more robust, though this is an edge case.

No security or performance concerns. Python test coverage looks adequate for both entry points. The insert_many and graph-batch argument (they only call begin() when nothing is active) is reasonable.

LGTM, with the optional hardening in point 5.

[claude]

Review of #10032 (Python binding: transaction ownership by stack mark)

Summary: Deciding ownership from the transaction stack captured before begin(), instead of from begun/was_active flags, closes the #9970 window where an interrupt after a nested push left a stray level. The change is small and the shared helper is easier to reason about than the old flags. Both callers (run_in_transaction and TransactionContext.__enter__) now use the same logic.

What looks good

• _end_transactions_opened_since bounds its loop by the depth found at call time, so a rollback that pops nothing can't spin. A failing rollback also doesn't skip the levels beneath it, and the first error is re-raised afterwards.
• The not was_active clause covers the non-nested case, where begin() reuses the base context and the depth doesn't grow. The PR description explains this well.
• The tests cover the right scenarios: interrupt after a nested begin, a refusing begin, a failing block inside a caller's transaction, a failed nested commit, and a rollback that fails after popping. The fixture teardown (rollbackAllNested) stops one failed assertion from cascading into the other tests.

Minor suggestions (non-blocking)

1. _transaction_stack_mark makes two JNI calls on every run_in_transaction attempt and every with db.transaction(). That is cheap, but it sits on a hot path for small transactions. If it ever shows up in a profile, getNestedTransactions() alone may be enough, because is_transaction_active() could be derived from it.
2. mark is taken outside the try in run_in_transaction. That is fine today, since the mark itself can't leave a level open. A short comment saying so would stop someone later moving begin() above it.
3. In _end_transactions_opened_since, the final if not was_active and self.is_transaction_active(): self.rollback() can raise before first_error is re-raised, which would mask the earlier error. Both callers swallow Exception, so this only affects the error that gets lost. Wrapping it in the same try/except would make it consistent.
4. The interrupt tests monkeypatch Database.begin. That simulates the interrupt well, but it doesn't exercise a real async KeyboardInterrupt. That is probably acceptable given how hard it is to test deterministically.

Style: The code follows the surrounding conventions, and I saw no new dependencies, System.out calls or security concerns. I didn't run the tests locally.

Overall, LGTM with the optional nits above.

[claude]

Review

Overall this is a sound fix. Deciding ownership from the transaction stack as it was before begin(), instead of from a begun flag set after it, closes the #9970 window. Having one helper shared by run_in_transaction and TransactionContext.__enter__ is cleaner than the two ad-hoc checks it replaces. I only read the diff and did not run the tests.

Correctness

• The bounded loop (getNestedTransactions() - depth_before) cannot spin forever, and it keeps going past a failing rollback. The range is empty when the result is negative, which is fine.
• The follow-up not was_active and is_transaction_active() check correctly covers the case where begin() reuses the base context and the depth stays flat.
• Minor: _end_transactions_opened_since calls getNestedTransactions() (a JNI call) inside the cleanup path. If that call raises, the except Exception in the caller swallows it and the second clause is skipped, so a transaction opened from a non-active state can stay open. Wrapping the depth read in its own try, or falling back to the was_active rollback, would make cleanup a bit more robust. This is unlikely in practice.
• Minor: raise first_error outside the original except context loses the chain. Both callers swallow it with except Exception anyway, so it only matters for BaseException subclasses, and those are not caught by the inner except Exception in the loop. If an interrupt arrives mid-cleanup, the remaining levels are not unwound. That is probably unavoidable.
• The behaviour change is called out in the PR description: after a failed nested commit, the caller's level is no longer rolled back. It is the more correct semantics, and test_a_failed_nested_commit_leaves_the_callers_transaction_open pins it. A release note would help, since someone may rely on the old behaviour.

Style and design

• transactions.py calls two private Database helpers. They are in the same package, so this is acceptable. Making them non-underscore, or exposing a small context-manager helper, would be tidier if a third caller appears.
• The comments are long compared with the surrounding code, but they explain non-obvious ordering (why the mark sits above begin()), so they are worth keeping.

Performance

• This adds one extra JNI call per run_in_transaction attempt and per with db.transaction(). That is negligible next to begin() itself.

Tests

• Coverage is good. It includes the nested interrupt, the refused begin(), a failing block, a failed nested commit, and a failing rollback that must not skip lower levels. The teardown fixture also keeps a failed assertion from cascading into drop().
• The interrupt is simulated by monkeypatching begin, which is reasonable because the window cannot be hit deterministically with a real signal. The # nosec B110 annotations are consistent with the rest of the binding.
• I have no blocking concerns. This looks good to merge once CI is green.

🤖 Generated with Claude Code

[claude]

Review

The ownership rule is sound: comparing the stack against a mark taken before begin() closes the #9970 window. It also explains the base-context reuse case, where depth stays 1 but a transaction becomes active, and that is why the helper has two clauses. No blocking issues. I read the diff only and did not run the tests.

Strengths

• One helper serves both callers and the begun flag is gone.
• The rollback loop is bounded, and a failing rollback no longer skips the levels below it.
• Each behaviour change has a regression test, including the failed-nested-commit case.

Non-blocking notes

1. _end_transactions_opened_since re-raises first_error, but both callers wrap it in except Exception: pass. That keeps the old best-effort behaviour, so the raise only matters to direct callers. A one-line docstring note, or logging at debug level, would make the intent clear.
2. TransactionContext calls the private Database._transaction_stack_mark and _end_transactions_opened_since. That is fine within the package, but the _FakeDatabase in test_transaction_context_interrupt.py now re-implements _end_transactions_opened_since. The fake can drift from the real helper, so these tests cover TransactionContext against the fake and not the real logic. The real-database tests in test_interrupt_hygiene.py cover that, which is acceptable.
3. Performance: run_in_transaction now makes one extra JNI call per attempt (getNestedTransactions), plus a second one on the failure path. That is negligible.
4. Behaviour change: a block that fails after its own nested level was already popped (a failed nested commit) no longer rolls back the caller's level. This is the correct ownership rule and the PR documents it. A release-notes entry would help, since some users may have relied on the old behaviour.
5. The tests simulate the interrupt by patching begin, which the PR acknowledges. They are deterministic and cover the logic.

LGTM.