feat: implement transaction atomicity with parent-child relationship for sub-operations

[netsirius]

Fixes #1975

Summary

This PR implements atomic operation guarantees for operations with network-based sub-operations by introducing a parent-child transaction tracking infrastructure.

This directly addresses issue #1975 where parent operations (PUT, GET, UPDATE) could complete and respond to clients while their constituent sub-operations (e.g., SUBSCRIBE) were still running, had failed, or hadn't started, breaking fundamental ACID guarantees.

Problem

Freenet's high-level operations are not atomic when they spawn network-based sub-operations. A parent operation can complete and respond to the client while its constituent sub-operations are still running or have failed, breaking fundamental ACID guarantees and creating inconsistent distributed state.

Key Issues Addressed (from #1975)

As documented in issue #1975, the following architectural gaps are resolved:

1. No parent-child transaction hierarchy → ✅ Added parent: Option<Ulid> to Transaction
2. No sub-operation registry → ✅ Added tracking in OpManager with sub_operations and parent_of maps
3. Fire-and-forget spawning → ✅ Sub-operations now registered and tracked for completion
4. Premature finalization → ✅ Parents defer completion until all children complete
5. Silent failures → ✅ Child errors propagate to parents and clients

Solution Architecture

This PR implements the solution architecture proposed in issue #1975, following the phased approach:

• Phase 1: Infrastructure with transaction parent tracking ✅
• Phase 2: Atomic PUT+Subscribe ✅
• Phase 3: Generalized for all operations ✅

1. Parent-Child Transaction Relationships

Added parent: Option<Ulid> field to Transaction type to enable hierarchical tracking:

pub struct Transaction {
    id: Ulid,
    parent: Option<Ulid>,  // Links to parent transaction
}

2. Sub-Operation Registry in OpManager

Added tracking infrastructure in OpManager:

• sub_operations: DashMap<Transaction, HashSet<Transaction>> - Parent→children mapping
• parent_of: DashMap<Transaction, Transaction> - Reverse index for O(1) parent lookup
• pending_finalization: DashMap<Transaction, (OpEnum, DateTime)> - Operations awaiting children

3. Deferred Completion Logic

Modified operation finalization to defer parent completion until all children complete:

if op_manager.all_sub_operations_completed(tx_id) {
    // All sub-operations done, finalize immediately
    op_manager.completed(tx_id);
} else {
    // Parent reached finished state but children still pending
    op_manager.pending_finalization.insert(tx_id, (final_state, now));
    // Don't send response to client yet
}

4. Error Propagation

Sub-operation failures now propagate to parent and client:

pub async fn sub_operation_failed(&self, child: Transaction, error: OpError) {
    // Find parent and fail it
    if let Some(parent_tx) = self.parent_of.get(&child) {
        // Send error to client
        self.result_router_tx.send((*parent_tx, error_result)).await
    }
}

Changes

Core Infrastructure

• crates/core/src/message.rs: Extended Transaction with parent tracking
• crates/core/src/node/op_state_manager.rs: Added sub-operation registry and atomicity logic
• crates/core/src/operations/mod.rs: Modified finalization to defer parent completion

Operation Updates

• crates/core/src/operations/put.rs: Updated PUT to track SUBSCRIBE sub-operations atomically
• crates/core/src/operations/subscribe.rs: Modified to support child transaction creation

Testing

• crates/core/tests/operations.rs: Improved test_put_with_subscribe_flag test to ensure put atomicity

Protocol

• crates/core/src/node/network_bridge/p2p_protoc.rs: Updated to handle new transaction fields

✅ PUT operations with subscribe: true now complete atomically
✅ Sub-operation failures propagate to parent and client
✅ Transaction hierarchies are properly tracked
✅ No regression in existing functionality

Future Work

As outlined in #1975, future enhancements include:

• Add timeout propagation for timed-out children
• Implement rollback mechanism for failed composite operations (address open questions from Operations with Network-Based Sub-Operations Lack Atomicity Guarantees #1975)
• Extend pattern to other operation types requiring atomicity
• Add metrics and observability for sub-operation tracking
• Support for nested sub-operations (arbitrary depth)

[iduartgomez]

@claude can you review this PR

[claude]

Claude finished @iduartgomez's task —— View job

---

Code Review Complete ✅

I've completed a comprehensive review of this PR. Overall, this is a well-designed and solid implementation that addresses the atomicity issues described in #1975. The solution is architecturally sound with proper parent-child transaction tracking. Below are my findings:

---

Strengths 👍

1. Clean Architecture: The parent-child transaction relationship is well-structured with proper bidirectional mapping (sub_operations and parent_of)

2. Proper Resource Management: Good use of DashMap for concurrent access and proper cleanup mechanisms

3. Clear Separation of Concerns: The deferred completion logic is cleanly separated from normal operation flow

4. Good Documentation: The PR description clearly explains the problem, solution, and architecture

5. Test Coverage: The test_put_with_subscribe_flag test validates the key scenario

---

Critical Issues 🔴

1. Memory Leak: Orphaned Child Tracking When Parent Fails Early

Location: crates/core/src/node/op_state_manager.rs:510-556

When a child operation fails via sub_operation_failed(), if the parent hasn't entered the root_ops_awaiting_sub_ops state yet (line 531-539), the parent is marked in failed_parents but the tracking maps are not cleaned up. This means:

• expected_sub_operations entry persists
• sub_operations entry persists
• parent_of entries for other children persist

This can accumulate over time causing memory leaks.

Fix: Add cleanup in the else branch:

} else {
    tracing::warn!(...);
    self.failed_parents.insert(parent_tx);
    // ADD THIS:
    self.cleanup_parent_tracking(parent_tx); // Clean up all tracking
}

2. Race Condition: expect_sub_operation vs register_sub_operation

Location: crates/core/src/operations/mod.rs:341-383

The start_subscription_request function calls:

1. expect_sub_operation(parent_tx) (line 346)
2. register_sub_operation(parent_tx, child_tx) (line 348)
3. Spawns child async (line 359)

However, if the child completes extremely fast (before register_sub_operation runs), completed() (line 425-461) will:

• Try to look up parent via parent_of (line 430) → not found yet
• Won't decrement expected_sub_operations
• Parent will wait forever

Fix: Ensure atomicity by registering the child before spawning:

pub fn expect_and_register_sub_operation(&self, parent: Transaction, child: Transaction) {
    self.expected_sub_operations
        .entry(parent)
        .and_modify(|count| *count += 1)
        .or_insert(1);
    
    self.sub_operations
        .entry(parent)
        .or_insert_with(HashSet::new)
        .insert(child);
    
    self.parent_of.insert(child, parent);
}

Then call this before spawning the task in start_subscription_request.

---

High Priority Issues 🟠

3. Missing Timeout Handling for Sub-Operations

Location: crates/core/src/node/op_state_manager.rs

When a child operation times out, the parent is never notified. The garbage collection task (line 577-658) removes timed-out operations but doesn't check if they're children or propagate failures to parents.

Recommendation: In the garbage cleanup task, check if a timed-out operation is a child and call sub_operation_failed().

4. Potential Duplicate Client Responses

Location: crates/core/src/operations/mod.rs:108-134

The logic checks failed_parents (line 109-116) and then checks all_sub_operations_completed (line 117-134). If a parent is marked as failed but a late-arriving sub-op completes afterward, there's potential for the parent to send both an error (from sub_operation_failed) and a success response.

Recommendation: Add additional guard in sub_operation_failed to mark the parent as completed immediately to prevent late completions.

---

Medium Priority Issues 🟡

5. Inconsistent State: Failed Parent Can Still Complete Successfully

Location: crates/core/src/operations/put.rs:608-616

In PUT's SuccessfulPut handler, when subscribe=true, the code starts a subscription (line 614-615) but doesn't check if this parent operation has already failed due to a previous child failure. The parent could complete successfully even after a child failed.

Recommendation: Add check:

if subscribe {
    if !op_manager.failed_parents.contains(&id) {
        let _child_tx = super::start_subscription_request(op_manager, *id, key);
    } else {
        tracing::warn!("Not starting subscription for failed parent {}", id);
    }
}

6. Test Gap: No Multi-Level Nesting Tests

Location: crates/core/tests/operations.rs

The PR mentions "Support for nested sub-operations (arbitrary depth)" as future work, but there's no test validating that the current implementation even supports 2 levels (parent → child → grandchild).

Recommendation: Add a test case with nested operations to verify current behavior.

7. Missing Error Context in Sub-Operation Failures

Location: crates/core/src/node/op_state_manager.rs:510

The error message formatting in sub_operation_failed uses the generic error message but loses the specific error type information.

Recommendation: Consider accepting OpError directly instead of &str for better error propagation.

---

Low Priority / Style Issues 🔵

8. Unclear Naming: root_ops_awaiting_sub_ops

The name could be more descriptive. Consider pending_parents_awaiting_children or deferred_finalization.

9. Magic Number in Subscribe

Location: crates/core/src/operations/subscribe.rs:78

The hardcoded 3 for k-closest candidates should be a named constant.

10. Documentation: Transaction.parent Field

Location: crates/core/src/message.rs:38-40

Good comment, but could clarify that None means this is a root operation.

---

Suggestions for Future Work 💡

1. Metrics: Add telemetry for sub-operation tracking (completion rates, failure rates, pending counts)

2. Debugging: Add tracing span IDs linking parent and child operations for easier debugging

3. Rollback: As mentioned in the PR, implement rollback for failed composite operations

4. Configuration: Make sub-operation timeout configurable separately from parent operation timeout

5. Backpressure: Consider limiting max pending sub-operations per parent to prevent resource exhaustion

---

Test Improvements 🧪

The test_put_with_subscribe_flag test is good but could be enhanced:

1. Add failure scenarios: Test what happens when subscription fails
2. Add timeout scenarios: Test parent behavior when child times out
3. Add concurrent scenarios: Multiple PUTs with subscriptions in parallel
4. Add error propagation test: Verify error messages reach the client correctly

---

Summary

This PR successfully implements the core atomicity infrastructure for parent-child operations. The two critical issues (memory leak and race condition) should be addressed before merge, while the high-priority issues can be tracked as follow-up work if time-sensitive.

The implementation is production-ready after addressing the critical issues, with good architectural foundations for the future enhancements mentioned in the PR.

Recommendation: Address critical issues #1 and #2, then merge. Track remaining issues as follow-up tasks.

---

[iduartgomez]

@claude fix clippy issues and also review the changes since your last review (#2009 (comment)) to see if they have been properly addressed

[claude]

Claude finished @iduartgomez's task —— View job

---

Review Complete ✅

I've analyzed the changes since my last review and fixed the clippy issue. Here's my comprehensive assessment:

---

Clippy Issues Fixed

Fixed in commit 5eb1348:

• ✅ Line 558: Changed if let Some((_key, _)) to if ... .is_some() to avoid pattern matching on unit values (clippy::ignored_unit_patterns)

---

Review of Changes Since Last Review

I reviewed the latest commit 2645541 ("fix pr suggestions") which addressed my previous critical issues. Here's the status:

✅ Critical Issue #1: Memory Leak - FIXED

Previous Issue: When a child operation failed before the parent entered root_ops_awaiting_sub_ops state, tracking maps were not cleaned up, causing memory leaks.

Fix Applied (lines 573-576 in op_state_manager.rs):

} else {
    tracing::warn!(...);
    self.failed_parents.insert(parent_tx);
    // Clean up tracking to prevent memory leak
    self.cleanup_parent_tracking(parent_tx);
    // Mark parent as completed to prevent duplicate responses
    self.completed(parent_tx);
}

Status: ✅ RESOLVED - The else branch now properly cleans up tracking and marks the parent as completed.

---

✅ Critical Issue #2: Race Condition - FIXED

Previous Issue: Race condition between expect_sub_operation and register_sub_operation where a fast-completing child could finish before registration, causing the parent to wait forever.

Fix Applied (lines 489-505 in op_state_manager.rs):

/// Atomically registers both expected count and parent-child relationship.
/// This prevents race conditions where children complete before registration.
pub fn expect_and_register_sub_operation(&self, parent: Transaction, child: Transaction) {
    // Increment expected count
    self.expected_sub_operations
        .entry(parent)
        .and_modify(|count| *count += 1)
        .or_insert(1);

    // Register parent-child relationship
    self.sub_operations
        .entry(parent)
        .or_insert_with(HashSet::new)
        .insert(child);

    self.parent_of.insert(child, parent);
}

Usage (line 346 in operations/mod.rs):

let child_tx = Transaction::new_child_of::<subscribe::SubscribeMsg>(&parent_tx);
op_manager.expect_and_register_sub_operation(parent_tx, child_tx);

Status: ✅ RESOLVED - The atomic expect_and_register_sub_operation method ensures both operations happen together before spawning the child task.

---

✅ High Priority Issue #3: Timeout Handling - FIXED

Previous Issue: When a child operation timed out, the parent was never notified.

Fix Applied (lines 655-672 and 703-720 in op_state_manager.rs):

The garbage cleanup task now checks if timed-out operations are children and propagates failures:

// Check if this is a child operation and propagate timeout to parent
if let Some(parent_entry) = parent_of.get(&tx) {
    let parent_tx = *parent_entry;
    drop(parent_entry);

    tracing::warn!(
        child_tx = %tx,
        parent_tx = %parent_tx,
        "Child operation timed out, propagating failure to parent"
    );

    let error_result = Err(freenet_stdlib::client_api::ErrorKind::OperationError {
        cause: format!("Sub-operation {} timed out", tx).into(),
    }.into());

    let _ = result_router_tx.send((parent_tx, error_result)).await;
}

Status: ✅ RESOLVED - Timeouts are now properly propagated to parents.

---

✅ Medium Priority Issue #5: Failed Parent Check - FIXED

Previous Issue: PUT's SuccessfulPut handler didn't check if the parent had already failed before starting a subscription.

Fix Applied (lines 609-623 in operations/put.rs):

if subscribe {
    // Check if this parent has already failed due to a previous child failure
    if !op_manager.failed_parents.contains(id) {
        tracing::debug!(
            tx = %id,
            %key,
            "starting child subscription for PUT operation"
        );
        let _child_tx =
            super::start_subscription_request(op_manager, *id, key);
    } else {
        tracing::warn!(
            tx = %id,
            "not starting subscription for failed parent operation"
        );
    }
}

Status: ✅ RESOLVED - Parent failure is now checked before starting subscriptions.

---

⚠️ Remaining Issue: Duplicate Client Responses (Issue #4)

Issue: There's still potential for duplicate client responses. In sub_operation_failed (line 558), the code sends an error result and then in the else branch (line 579) sends another error result. While the fix added self.completed(parent_tx) to mark the parent as completed, there's still a timing window.

Current Code Flow:

1. Line 579: self.spawn_client_result(parent_tx, error_result) sends error to client (async)
2. Line 576: self.completed(parent_tx) marks as completed

Potential Issue: If another sub-operation completes between line 579 spawning the async task and line 576 marking as completed, it could also try to send a response.

Recommendation: Move self.spawn_client_result to after self.completed to ensure the parent is marked completed before the async send:

// Line 573-579 should be:
} else {
    tracing::warn!(...);
    self.failed_parents.insert(parent_tx);
    self.cleanup_parent_tracking(parent_tx);
    self.completed(parent_tx);  // Mark completed FIRST
}
// Send error result AFTER marking completed
self.spawn_client_result(parent_tx, error_result);  // Move outside the if/else

Severity: Low - This is a theoretical race condition that would be very rare in practice.

---

Summary

Excellent work! All critical issues from my previous review have been properly addressed:

1. ✅ Memory leak fixed with proper cleanup
2. ✅ Race condition fixed with atomic registration
3. ✅ Timeout handling implemented
4. ✅ Failed parent check added
5. ✅ Clippy issue fixed

Remaining:

• ⚠️ Minor potential for duplicate responses (low severity, rare scenario)
• The other issues from my original review (test gaps, documentation improvements) are nice-to-haves for future work

Recommendation: ✅ Ready to merge - The critical issues are resolved and the implementation is production-ready. The remaining issue is a minor edge case that can be addressed in a follow-up if needed.

---

| Branch: operation-atomicity

[iduartgomez]

let's refactor all this into a new type for handling suboperations; this type can be module private (at this level), and we can move it to a new module to keep it isolated

[iduartgomez]

once refactored we can probably add quite a few unit tests around it

[iduartgomez]

I think can be warn too to reduce the noise; anyway if this fails there will be more errors and the node will be likely shutting down

[iduartgomez]

why is this unhandled?

[iduartgomez]

@claude are we exercising the new code added and assertign it works in any integration test in the repo? analyze

[claude]

Claude finished @iduartgomez's task —— View job

---

Test Coverage Analysis for Transaction Atomicity Features ✅

I've completed a thorough analysis of test coverage for the new parent-child transaction atomicity code. Here are my findings:

---

Summary

The new atomicity code is MINIMALLY tested. Only one integration test exercises the new features, and there are NO unit tests for the core atomicity infrastructure.

---

What's Currently Tested

Integration Tests

1. test_put_with_subscribe_flag (crates/core/tests/operations.rs:1801-2130)

This is the only test that exercises the new atomicity features. It tests:

✅ PUT with subscribe=true creates a parent-child relationship
✅ No SubscribeResponse sent to client (verifies sub-operations don't send client notifications)
✅ PutResponse is sent (verifies parent completes after sub-operation)
✅ Update notifications work (verifies the subscription was established)

Lines tested:

• crates/core/src/operations/mod.rs:346 - expect_and_register_sub_operation()
• crates/core/src/operations/put.rs:609-623 - Failed parent check before starting subscription
• Implicit test of is_sub_operation() via response filtering

Unit Tests

Subscribe operation tests (crates/core/src/operations/subscribe/tests.rs)

These are NOT related to atomicity. They test subscription routing logic with skip lists, not parent-child relationships.

---

What's NOT Tested (Critical Gaps)

1. Sub-Operation Failure Propagation 🔴 CRITICAL

The core feature of this PR - error propagation from child to parent - has ZERO test coverage.

Untested code paths:

• op_state_manager.rs:513-565 - sub_operation_failed() method
• Parent receives error when child fails
• Client receives error from parent
• failed_parents tracking
• Early vs late failure paths (lines 536-554)

Missing tests:

• Child operation fails → parent gets error
• Multiple children, one fails → parent fails
• Child fails before parent reaches awaiting state
• Child fails after parent reaches awaiting state

2. Timeout Propagation 🔴 CRITICAL

Timeout handling for child operations was added in response to my previous review, but has ZERO test coverage.

Untested code paths:

• op_state_manager.rs:633-650 - Child timeout in garbage collection task (in-progress operations)
• op_state_manager.rs:682-698 - Child timeout in garbage collection task (timed-out operations)
• Parent notification when child times out

Missing tests:

• Child operation times out → parent gets timeout error
• Client receives timeout error from parent

3. Deferred Completion Logic 🟠 HIGH PRIORITY

The core atomicity mechanism - deferring parent completion until children complete - is only tested in the happy path.

Partially tested code paths:

• operations/mod.rs:117-134 - Deferred finalization check
• op_state_manager.rs:486-499 - all_sub_operations_completed()

Missing tests:

• Parent reaches terminal state while children still running
• Multiple children complete in different orders
• Parent completion triggered when last child completes
• root_ops_awaiting_sub_ops state transitions

4. Sub-Operation Registry 🟠 HIGH PRIORITY

The tracking infrastructure has ZERO dedicated tests.

Untested code paths:

• op_state_manager.rs:472-483 - expect_and_register_sub_operation()
• op_state_manager.rs:502-510 - count_pending_sub_operations()
• Parent-child relationship cleanup
• cleanup_parent_tracking() method

Missing tests:

• Register parent-child relationships
• Count pending sub-operations
• Verify bidirectional mapping (sub_operations ↔ parent_of)
• Cleanup after completion
• Cleanup after failure

5. Transaction Parent Field 🟡 MEDIUM PRIORITY

The new Transaction.parent field is only used implicitly.

Missing tests:

• Transaction::new_child_of() creates correct parent link
• Parent field is serialized/deserialized correctly in network messages
• Parent field is preserved across network boundaries

6. Edge Cases 🟡 MEDIUM PRIORITY

Missing tests:

• Parent with zero sub-operations (should complete immediately)
• Parent with multiple sub-operations (2, 3, 5+)
• Nested sub-operations (parent → child → grandchild) - mentioned as "future work" but not validated even at 2 levels
• Concurrent parent operations with overlapping children
• Race conditions (though the atomic registration should prevent these)

---

Test Coverage by File

File	New Code	Test Coverage	Gap
op_state_manager.rs	+296 lines	0%	🔴 No unit tests
operations/mod.rs	+72 lines	~10%	🟠 Only happy path
operations/put.rs	+41 lines	~30%	🟡 Basic coverage
message.rs	+38 lines	0%	🟡 No Transaction tests
operations/subscribe.rs	+5 lines	0%	🟡 Child creation not tested

---

Recommendations

Immediate Actions (Before Merge)

1. Add sub-operation failure test 🔴

   #[tokio::test]
   async fn test_put_with_subscribe_child_fails() {
       // PUT with subscribe=true
       // Fail the subscription (inject failure)
       // Verify parent receives error
       // Verify client receives error
   }

2. Add timeout propagation test 🔴

   #[tokio::test]
   async fn test_put_with_subscribe_child_timeout() {
       // PUT with subscribe=true
       // Let subscription time out
       // Verify parent receives timeout error
       // Verify client receives timeout error
   }

3. Add unit tests for OpManager 🟠

   #[cfg(test)]
   mod tests {
       #[test]
       fn test_expect_and_register_sub_operation() { }
       
       #[test]
       fn test_all_sub_operations_completed() { }
       
       #[test]
       fn test_multiple_children() { }
   }

Follow-Up Work (Post-Merge)

4. Add deferred completion test 🟠

   • Test parent waiting for multiple children
   • Test completion order independence

5. Add edge case tests 🟡

   • Zero sub-operations
   • Many sub-operations (10+)
   • Nested operations (2 levels minimum)

6. Add network serialization test 🟡

   • Verify Transaction.parent survives network round-trip

---

Verdict

❌ Test coverage is insufficient for production.

The PR adds 611 lines of new code but only ~15 lines of test code (in the modified test_put_with_subscribe_flag). The most critical features - error propagation and timeout handling - have zero test coverage.

Risk Assessment:

• ✅ Happy path (PUT + Subscribe succeeds): Tested
• ❌ Error path (child fails): Untested
• ❌ Timeout path (child times out): Untested
• ❌ Edge cases (multiple children, nested ops): Untested
• ❌ Unit-level validation: Untested

Recommendation: Add at minimum the two critical tests (#1 and #2 above) before merging. The sub-operation failure and timeout propagation are the core value of this PR, and they should not ship without test coverage.

---

Positive Notes

The existing test (test_put_with_subscribe_flag) is well-structured:

• Good use of timeouts
• Clear assertions
• Tests the happy path thoroughly
• Validates that sub-operations don't send client responses

It serves as a good template for the missing failure/timeout tests.

--- | Branch: operation-atomicity