GET operation fails when caching contracts with version-based update validation

[sanity]

Bug Description

GET operations fail when attempting to cache contracts that implement version-based state validation (like web-container-contract), if the contract is already cached locally with the same version.

Steps to Reproduce

1. Publish a contract using web-container-contract (which implements version checking in its update_state() method)
2. Contract gets cached locally via PUT operation
3. Perform a GET operation for the same contract
4. GET retrieves contract from network successfully
5. GET attempts to cache it locally via PutQuery
6. Fails with: invalid contract update, reason: New state version X must be higher than current version X

Root Cause

In crates/core/src/operations/get.rs:970-1046, when GET receives a contract from the network, it determines if it should cache locally:

let should_put = if is_original_requester && subscribe_requested {
    true
} else {
    op_manager.ring.should_seed(&key)
};

If caching is needed, it calls PutQuery which triggers the contract's update_state() method. For contracts like web-container-contract that enforce strict version ordering:

if metadata.version <= current_version {
    return Err(ContractError::InvalidUpdateWithInfo {
        reason: format!(
            "New state version {} must be higher than current version {}",
            metadata.version, current_version
        ),
    });
}

This rejects the update because the network version equals the local cached version.

Error Log

ERROR freenet::server::path_handlers: error getting contract: client error: 
error while executing operation in the network: update error for contract U6odK7aUsr9QXM9oQrUMuyUEyqDBzYBsFmYbLcQSwgY, 
reason: execution error: invalid contract update, reason: 
New state version 29361237 must be higher than current version 29361237

Context

This bug was exposed after PR #1996 fixed state persistence after PUT merge. Previously, the local cache might have been stale, so the version conflict didn't occur as frequently.

Note: "version" is not a Freenet protocol concept - it's implemented by individual contracts (like web-container-contract) in their state and validation logic.

Proposed Solutions

1. GET operation check: Before attempting PutQuery, check if local cached state matches network state (skip caching if identical)
2. Contract-side fix: Modify contracts to allow idempotent updates (equal versions accepted if state is identical)
3. Alternative caching mechanism: GET should use a different method that doesn't trigger update_state() validation for simple caching

Version

Freenet v0.1.35

Related Issues/PRs

• Contract state not persisted after PUT merge in network mode #1995 - State persistence issue (fixed by fix: persist contract state after PUT merge in upsert_contract_state #1996)
• fix: persist contract state after PUT merge in upsert_contract_state #1996 - Fix persist contract state after PUT merge

[sanity]

Additional Design Issue

The GET operation successfully retrieves the contract from the network, but then fails the entire operation because an optional caching step fails.

Current Flow

1. GET retrieves contract from network ✅ (data retrieved successfully)
2. GET decides to cache locally (for DHT seeding or subscription)
3. PutQuery fails ❌ (version check in contract's update_state() rejects it)
4. Entire GET operation fails ❌ (returns error to client)

Code Reference

In get.rs:997-1003:

ContractHandlerEvent::PutResponse {
    new_value: Err(err),
} => {
    if is_original_requester {
        // Original requester, return error
        tracing::debug!(tx = %id, error = %err, "Failed put at executor");
        return Err(OpError::ExecutorError(err));
    }

Problem

The client never receives the data even though it was successfully retrieved from the network. The caching is an optimization for the DHT, not critical to fulfilling the GET request.

Better Behavior

When local caching fails:

• Log a warning
• Still return the retrieved data to the client
• GET should only fail if it cannot retrieve from the network

The current behavior treats an optimization failure as a critical error, making GET operations fail unnecessarily.

[sanity]

Fix Implemented

Created PR #2020 to address both the immediate issue and the underlying design problem.

Two-Layer Solution

Layer 1: Prevention

• Check local state before attempting PutQuery
• Skip caching entirely if states match (avoids triggering version validation)

Layer 2: Resilience

• If PutQuery fails for any reason, log warning but continue
• Return successfully retrieved data to client
• Caching is an optimization - shouldn't fail GET operations

Code Changes

Modified crates/core/src/operations/get.rs:970-1081 to:

1. Query local state via GetQuery before attempting cache
2. Compare local vs incoming state bytes
3. Skip PutQuery when states are identical
4. Handle PutQuery failures gracefully with warning instead of error

The fix ensures GET operations succeed when data is retrieved, regardless of optional caching step outcomes.

[AI-assisted debugging and comment]

[sanity]

Test Coverage Gap Analysis

@sanity raised a critical question: Why didn't our tests catch these fundamental issues?

The Problem

Several integration tests that SHOULD catch GET operation failures are disabled:

#[ignore = "Test has reliability issues in CI - PUT operations timeout and gateway crashes"]
async fn test_small_network_get_failure()

#[ignore = "Test has never worked - gateway nodes fail on startup with channel closed errors"]
async fn test_app_run_2_gateways_2_nodes()

#[ignore = "Test has never worked - nodes fail on startup with channel closed errors"]  
async fn test_partially_connected_network()

Why Tests Didn't Catch This

1. Tests are ignored/disabled - The tests that would exercise GET with caching are marked #[ignore]
2. Tests themselves are broken - Comments indicate tests "never worked" or have "reliability issues"
3. No unit tests for edge cases - No specific unit test for "GET with idempotent state caching"
4. Integration tests too coarse - When they work, they may pass even with the bug (if they don't hit the edge case)

The Vicious Cycle

Bug exists → Test exposes bug → Test disabled ("reliability issues")
→ Bug not fixed → More bugs accumulate → Tests stay disabled

Recommendations

1. Fix the tests first - Invest time to make ignored tests work
2. Add targeted unit tests - Specifically for edge cases like:
   • GET with identical cached state
   • GET with cache failure (should not fail operation)
   • Contract update_state rejection scenarios
3. Never ignore tests without TODO-MUST-FIX - If a test is broken, it should block commits
4. Test pyramid - More unit tests (fast, specific), fewer integration tests (slow, broad)

Immediate Action

Should we:

• Create an issue to fix all ignored tests?
• Add a CI check that fails if tests are ignored without explicit approval?
• Write unit tests for this specific bug scenario?

[AI-assisted debugging and comment]