[malibio]

Summary

NodeService::move_children_to_parent committed the store's edge swap and the service's version bump in two separate transactions. A concurrent write to a child landing between them failed the RPC after the edges had moved: the frontend rolled back locally, the backend kept the children re-parented, and their RelationshipUpdated events never fired.

• SqliteStore::move_children_to_parent → move_children_to_parent_in_tx(&Tx, …). It no longer opens its own transaction. The write guard held by the Tx still covers the max-order read → write.
• The service composes the edge swap, refresh_for_rootness_in_tx, the per-child version bump, and event emission in one with_transaction. A conflict anywhere rolls back every edge, and events flush only after commit (ADR-069 §1a/§2).
• Store-level tests go through a small with_transaction helper.

Tests

• test_move_children_to_parent_bump_conflict_rolls_back_edge_swap: forces a bump conflict after the edge swap inside one transaction. The swap rolls back, the version is unchanged, and no event escapes. (Replaces the previous bump-loop-only F9 test.)
• test_move_children_to_parent_result_matches_backend_under_concurrent_write: races a real update_node against the real method on a multi-threaded runtime, 50 iterations. The call's result must always match the backend parent and event count. Against the old two-transaction code this fails within 1–2 iterations; with the fix, it passed 5 of 5 runs.
• cargo test -p nodespace-core --lib: 1761 passed. The pre-push gate passed.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review: PR #3029

Review Type: Initial Review (the PR had no earlier reviews or comments)

Summary

This is a small, focused fix that makes the codebase healthier. The store method becomes a pub(crate) _in_tx primitive. The service now runs the edge swap, title refresh, version bumps and buffered RelationshipUpdated events inside one NodeService::with_transaction, so the window between the two commits is gone. This matches ADR-069 §1a/§2 and the pattern the other tx callers already use. No backward-compat shims, no dead code, and no issue numbers in comments. The one caller of the old store method outside core was the service, and the daemon and Tauri callers are unchanged.

Requirements Check (#2950)

• ✅ Edge swap and version bumps commit in a single transaction. move_children_to_parent_in_tx runs on tx.store_tx() inside the service's with_transaction. The store's own transaction()/commit() calls were removed.
• ✅ A version conflict during the bump leaves every edge unmoved. Any Err from update_node_with_version_bump_in_tx rolls back the whole tx. test_move_children_to_parent_bump_conflict_rolls_back_edge_swap checks that the parent is unchanged, the version is unchanged, and no event was emitted.
• ✅ Events are emitted only after the single transaction commits. emit_event goes to the BatchState::Transactional buffer and is flushed after commit. The test checks rx.try_recv().is_err() on rollback.
• ✅ Regression test that forces the conflict. There are two tests: (a) a deterministic forced-conflict test and (b) a 50-iteration multi-thread race against the real method. The author reports that (b) fails within 1–2 iterations on the old code. On the new code its assertions hold under any interleaving, so it cannot flake into a false failure. Measured runtime: all 8 move_children_to_parent tests finish in 0.28s.

Code Review Findings

🟢 Suggestion: title refresh moved from best-effort to transactional. This is correct, not a regression.
📁 packages/core/src/services/node_service/hierarchy.rs:783-795
Context: the old code called refresh_for_rootness after commit, and a failure there was only logged. Now a refresh_for_rootness_in_tx error aborts the move.
Assessment: this is the right behavior and matches the other tx callers (relationship.rs:1097, relationship.rs:1422, conflicts.rs:305). The only errors that can now propagate are SQL failures of get_node_in_tx / set_title_in_tx on the same tx connection. A failure there means the transaction is already in trouble, so aborting is correct. compute_title cannot fail because of schema lookup: title_schema swallows and logs those errors. Committing edges with a stale title was the "only roots are results" invariant drift the tx twin exists to prevent. The embedding half is still deferred until after commit through tx.defer_embedding_refresh. No change needed.
Principle: atomicity and invariant preservation.

🟢 Suggestion: the VERSION_CONFLICT mapping still reports (0, 0) and parses a string.
📁 packages/core/src/services/node_service/hierarchy.rs:766-778 (the code moved; the behavior already existed)
Context: the store embeds "VERSION_CONFLICT: node '<id>' ..." and the service matches on that prefix. The error then reports expected=0, actual=0. The daemon maps any VersionConflict to Aborted (tested at daemon/src/services/node_service.rs:4093), so the frontend is unaffected.
Recommendation (optional, can be a follow-up): look up the conflicting child's expected version in children so at least expected is accurate. Better still, return a typed error from the store instead of matching on message text. Nothing new is broken here.
Principle: prefer typed errors over stringly-typed contracts.

🟢 Suggestion: no deadlock risk found.
Inside the closure, the only calls that are not _in_tx are compute_title(&node, Some(is_root)) and emit_event. Passing Some(is_root) skips the pooled-reader get_parent_id lookup. title_schema reads through the pooled reader, not the write guard. derive_title does no store I/O. emit_event only pushes to the batch buffer. None of these re-take the write guard.

🟢 Nit: redundant clones inside an FnOnce closure.
📁 hierarchy.rs:752-756
with_transaction takes an FnOnce, so children_owned, nodes and former_parents could be moved straight into the async move block instead of cloned. These are tiny vectors, and the clone-then-move shape matches other call sites, so this is optional.

🟢 Nit: the forced-conflict test rebuilds the production composition by hand.
📁 packages/core/src/services/node_service/mod.rs test_move_children_to_parent_bump_conflict_rolls_back_edge_swap
The test repeats the swap-then-bump sequence rather than calling move_children_to_parent, so it could drift from the production code. The race test does exercise the real method, so together they cover the requirement. Acceptable as it stands.

Recommendation: APPROVE

All four acceptance criteria are met. The change is atomic and consistent with ADR-069 and the existing _in_tx patterns, and the tests are fast and deterministic in what they assert. Nothing blocks the merge. The suggestions above are optional.

[malibio]

Automated pragmatic-code-review: APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.

[malibio]

Address-review summary

Review verdict was APPROVE with 🟢 suggestions only.

✅ Addressed (1): moved owned state straight into the FnOnce transaction closure and dropped the per-call clones (d3a3740).

⏭️ Skipped (4):

• Title refresh moving into the transaction: the reviewer confirmed this is correct and matches the other tx callers. No change.
• No deadlock risk: informational. No change.
• VERSION_CONFLICT reported as (0,0): this predates the PR, and the daemon maps every VersionConflict to Aborted. The right fix is a typed store error, which is a separate change.
• Forced-conflict test rebuilds the steps by hand: the multi-thread race test exercises the real method and catches the old bug, so drift is already covered.

Tests: 10/10 move_children tests pass, and the pre-push gate passed.

Re-review: not needed. The only code change is a clone removal with no logic change.