Give the event-batching slot ownership (closes #3217)

[malibio]

Closes #3217.

What

The single service-wide BatchState slot recorded what mode emission was in, not who owned it. This replaces it with owned buffers:

• Transaction buffer on NodeServiceTx. In-tx code emits via emit_event_in_tx(tx, …) → tx.buffer_event(..); plain emit_event and the store notifier never consult a transaction. A non-tx write notifying after releasing the write guard can no longer be buffered into (and discarded by) another task's rollback. No more Transactional state, parking, or ResetOnDrop.
• Owned batches. begin_batch_emit opens a batch keyed by a token and the opening tokio task (thread outside a task). Any number may be open; a node event joins its own task's most recent batch, else the most recently opened batch (so single-batch behaviour — and the sync page-guard contract — is unchanged). Each guard flushes only its own batch; the nested-guard debug_asserts are gone.
• Commit-ordered merge. with_transaction = run_transaction + publish_committed; a sequence number is reserved while the write guard is held, and a batch keeps the highest-seq event per node, so two commits publishing out of order still leave the later snapshot.

All in-transaction emit sites (crud/bulk/hierarchy/relationship/schema, incl. the four hierarchy emit_* helpers) now take tx.

Tests

• New: a_rolled_back_transaction_never_takes_another_writers_event, concurrent_batch_guards_each_deliver_only_their_own_events (both drop orders + same-task nesting), batch_keeps_the_later_commit_when_commits_publish_out_of_order.
• Dispatch ADR-060 invariant rules from every bulk write path #3210 regressions still pass (helper updated to emit_event_in_tx); bulk_invariant_dispatch_test 14/14.
• Full nodespace-core lib (1932) + integration tests pass; quality:fix clean.

Docs

ADR-069 §2 rewritten in nodespace-docs (45dad48) to describe the ownership model.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review: PR #3236 (closes #3217): give the event-batching slot ownership

Review Type

Initial review. The PR had no earlier reviews or comments.

Summary

The PR replaces the single service-wide BatchState slot (Immediate / Batching / Transactional { buf, parked }) with two things that each have an owner:

• Transaction events are buffered on the transaction's own NodeServiceTx through emit_event_in_tx, then published after commit by publish_committed.
• Batches are a Vec<OpenBatch>. Each batch has a token and an owner (Emitter: a tokio task id, or the thread id outside tokio). A node-keyed event joins the newest batch its own emitter opened. If that emitter has none open, it joins the newest batch overall. Each guard flushes only its own batch, found by token.

This removes a whole class of cross-task bugs rather than patching one. A plain write can no longer land in, or be discarded with, another task's transaction. The debug_asserts for nested or concurrent guards are gone because that state can no longer occur. The panic-reset guard (ResetOnDrop) is gone because a panic now just drops the transaction's own buffer. The change is smaller and easier to reason about than the parked-batch design it replaces. It is a clear net improvement.

Requirements Check

#	Criterion	Status
1	A non-transactional write's event is never buffered into or discarded by another task's transaction. Regression test uses a Notify-paused transaction that rolls back.	✅ a_rolled_back_transaction_never_takes_another_writers_event. The notified-late half fails on the old code.
2	Two concurrent guards each deliver exactly their own events in either drop order, with no debug_assert reachable.	✅ concurrent_batch_guards_each_deliver_only_their_own_events covers both drop orders, plus a nested guard. All debug_asserts are removed.
3	After two transactions on different tasks commit inside one open batch, the batch keeps the later commit's event for a shared node. Regression test.	✅ batch_keeps_the_later_commit_when_commits_publish_out_of_order. seq is reserved inside the store closure while the write guard is held, and Tx::with_transaction commits under that same guard, so seq order is commit order.
4	Existing regressions from the earlier bulk-dispatch work still pass.	✅ I ran the two named lib tests plus the batch suite locally (12/12 pass). The main agent reports that bulk_invariant_dispatch_test.rs passes.
5	ADR-069 §2 updated.	✅ nodespace-docs 45dad48
6	quality:fix clean; test:changed passes.	✅ as reported by the main agent

Verification of the key risk areas

• (a) No plain emit_event left inside a transaction. I checked all 16 remaining emit_event( call sites:

  • relationship.rs 179, 238, 311, 907, 962, 1006, 1961, 2077, 2204
  • schema.rs 1302–1317 (set_schema_relationships)
  • hierarchy.rs 1077 (bulk_create_has_child_edges)
  • markdown/mod.rs 1180
  • two test helpers

  Every one runs after a non-transactional store call. The transactional branches of create_relationship and delete_relationship return early into with_transaction before reaching those sites. Every _in_tx function and every with_transaction closure in the diff now uses emit_event_in_tx, including the helpers that now take tx: emit_move_events, emit_respread_events, emit_reorder_event and emit_former_parent_deleted. ✅

• (b) The sync-repo contract still holds. In nodespace-sync/.../reconnect.rs, each page guard is opened on the same task that calls bulk_create/bulk_update. publish_committed runs on the caller's task (Emitter::current()), so committed events join that page's own batch. The store notifier is synchronous on the writing task (SqliteStore::notify), so bulk_delete's events also join its own guard. The sync guards are sequential, not nested, so the new nesting semantics do not change them. ✅

• (c) Semantics compared with the old implementation:

  • Panic handling: strictly better. No global state is left to reset.
  • Ordering on commit: unchanged. With a batch open, node-keyed events are coalesced and relationship events broadcast immediately in order. With none open, everything broadcasts in emission order.
  • Cancellation: publish_committed runs synchronously right after run_transaction returns, so no .await sits between commit and publish.
  • Batch leakage: a transaction's events publish to whatever batch is open at commit, as before.

Code Review Findings

🔴 Critical

None.

🟡 Improvements

• 📁 packages/core/src/services/node_service/mod.rs:2959-2980 (emit_event / emit_event_in_tx)
  Nothing stops emit_event from being called inside a transaction. The old design buffered any emit_event inside a transaction automatically. Now, a future _in_tx function or with_transaction closure that calls self.emit_event(..) instead of emit_event_in_tx will broadcast an event before commit. That is exactly the ADR-069 §2 violation this PR guards against, and nothing would fail. The capture pattern (service_for_tx.emit_event(...)) makes this easy to write by accident.
  Suggestion: use a tokio::task_local! flag (for example IN_NODE_SERVICE_TX). Scope it around f(&ns_tx) in run_transaction, and add debug_assert!(!in_tx, "use emit_event_in_tx inside with_transaction") in emit_event. This is cheap, costs nothing in release builds, and turns a silent correctness bug into a test failure.
  Principle: poka-yoke, i.e. make the wrong call fail loudly. It matches the "unrepresentable by construction" discipline NodeServiceTx already follows.

• 📁 packages/core/src/services/node_service/mod.rs:149-164 (BatchState::route)
  A plain write reserves its seq after releasing the write guard, when its notifier fires. A transaction reserves its seq while holding the guard. This order is possible:

  1. Plain write B commits and releases the guard.
  2. Transaction A takes the guard, reserves seq=n, and commits.
  3. B's notifier runs and takes seq=n+1.

  If both touched the same node inside one open batch, the batch keeps B's older NodeUpdated snapshot over A's newer one. The window is very small, and this is not a regression: the old code did pure last-arrival-wins, which is worse. But the next_seq doc says "numbers follow commit order", which is only true between transactions.
  Suggestion: either narrow the doc on next_seq and with_transaction to "between transactions", or note the plain-write window. A full fix would reserve the seq inside the store's write path, which is not worth it here.
  Principle: documentation should state the guarantee that actually holds.

🟢 Nits

• Nit: 📁 mod.rs test batch_guard_dropped_during_a_transaction_leaves_the_slot_immediate. "Slot" and "Immediate" no longer exist. The issue's AC names this test, so renaming is optional; if you keep the name, the updated doc comment is enough.
• Nit: 📁 mod.rs test a_rolled_back_transaction_never_takes_another_writers_event. The doc says the queued create_node half covers the regression. That write acquires the guard only after the transaction has ended, so it would also pass on the old code. Only the notified-late half tells old and new apart. It is still a useful end-to-end check; consider softening the wording so it does not claim more than it tests.
• Nit: 📁 crud.rs:230-232. The rewrapped doc line (`self.emit_event_in_tx` — see `NodeService::with_transaction` — instead of relying on the store) is noticeably longer than the lines around it; rustfmt does not wrap comments.
• Nit: 📁 mod.rs BatchState::offer. The fallback to "the newest batch overall" means a user edit made during a sync replay coalesces into the sync page's batch and waits until that page flushes. This is unchanged from before and needed for batch_opened_during_a_transaction_receives_its_committed_events. Only mentioning it so the trade-off is on record; it is already described in the begin_batch_emit doc.

Recommendation

APPROVE. The design is correct and simpler, every acceptance criterion is met with tests that fail on the old code, and I found no transactional emit site that was missed. I'd recommend the task_local debug guard (🟡 #1) as a cheap safety net in this PR. The seq-doc precision (🟡 #2) and the nits are optional.

[malibio]

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

[malibio]

Review addressed

✅ 🟡 emit_event inside a transaction: a tokio::task_local! IN_TRANSACTION flag is scoped around the with_transaction closure; emit_event debug_assert!s it is unset. New emit_event_inside_a_transaction_is_refused (should_panic, debug-only). Full core lib (1933) + integration suites pass, so no existing path trips it.
✅ 🟡 next_seq doc: now states commit ordering holds between transactions, and describes the non-transactional window.
✅ Nit: a_rolled_back_transaction_never_takes_another_writers_event doc says which half distinguishes old/new behaviour.
✅ Nit: crud.rs doc line rewrapped.
⏭️ Rename of batch_guard_dropped_during_a_transaction_leaves_the_slot_immediate: kept, since the issue's acceptance criteria name it.
⏭️ offer fallback-to-newest-batch: unchanged behaviour, required by an existing regression; no change.

Re-review decision: NO RE-REVIEW NEEDED. Changes are a debug-only assertion with its own test plus doc wording; no production logic changed.