fix: make get operation resilient to local caching failures #2020

sanity · 2025-10-28T19:29:59Z

Summary

Fixes #2018

GET operations should not fail when local caching fails. The data was successfully retrieved from the network - caching is an optimization, not a requirement.

Problem

Two related issues caused GET operations to fail unnecessarily:

Redundant caching attempts: When GET retrieved a contract that was already cached locally with identical state, it would still attempt PutQuery, triggering version validation errors in contracts that enforce strict version ordering (like web-container-contract).
Caching treated as critical: When PutQuery failed, the entire GET operation would fail and return an error to the client, even though the data was successfully retrieved.

Solution

Layer 1: Prevention

Before attempting PutQuery, query local state and compare. If states match, skip caching entirely (just mark as seeded if needed).

Layer 2: Resilience

If PutQuery fails for any reason, log a warning but continue - return the successfully retrieved data instead of failing the GET.

Changes

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

Check local state before attempting cache via PutQuery
Skip redundant caching when states are identical
Handle PutQuery failures gracefully with warning instead of error

Testing

✅ Code compiles cleanly
✅ Pre-commit checks pass (fmt, clippy, TODO-MUST-FIX)
✅ Installed and ready for integration testing

Why This Fix is Correct

The GET operation's primary job is to retrieve data. Local caching is a DHT optimization that should never cause a GET to fail when the retrieval succeeded.

[AI-assisted debugging and comment]

🤖 Generated with Claude Code

Copilot

Pull Request Overview

This PR improves the GET operation's local caching logic by adding a state comparison check before attempting to cache received contracts. The change addresses issue #2018 by preventing version validation errors in contracts with strict version ordering requirements.

Key changes:

Adds a local state check before caching to detect if the network state already matches local state
Treats local caching failures as non-fatal warnings instead of operation errors
Simplifies error handling by removing the complex forward-error-to-requester logic

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

crates/core/src/operations/get.rs

Fixes #2018 ## Problem GET operations were failing when attempting to cache contracts that implement version-based state validation (like web-container-contract), even though the GET successfully retrieved the data from the network. ### Two Issues Fixed 1. **Redundant caching attempts**: When GET retrieved a contract from the network that was already cached locally with identical state, it would still attempt to cache via PutQuery. This triggered version validation in contracts that reject equal versions. 2. **Caching failure treated as critical error**: When local caching failed, the entire GET operation would fail and return an error to the client, even though the data was successfully retrieved from the network. Caching is an optimization for DHT seeding, not critical to fulfilling the GET request. ## Solution ### Layer 1: Prevention Before attempting PutQuery, check if local cached state matches the incoming state. If identical, skip caching entirely and just mark the contract as seeded if needed. ### Layer 2: Resilience If PutQuery fails for any reason, log a warning but continue with the GET operation. Return the successfully retrieved data to the client instead of treating the optional caching step as a critical error. ## Changes - `crates/core/src/operations/get.rs`: Modified GET operation to: - Query local state before attempting cache via PutQuery - Skip redundant caching when states match - Handle PutQuery failures gracefully by logging warning instead of failing the entire GET operation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

iduartgomez

Is this reproducible in a regression test?

sanity · 2025-10-29T14:10:52Z

@iduartgomez - Good question about regression tests!

Regression Test Status

Short answer: Not yet reproducible in automated tests, but this is a known systemic issue.

The Testing Problem

This bug should have been caught by existing integration tests like:

test_small_network_get_issue.rs::test_small_network_get_failure
run_app.rs::test_ping_multi_node
run_app.rs::test_ping_application_loop

However, these tests are all currently ignored/disabled. See Issue #2021 for full audit.

Why Tests Are Disabled

The ignore reasons reveal the underlying problems:

"Test has reliability issues in CI - PUT operations timeout and gateway crashes"
"Test has never worked - nodes fail on startup with channel closed errors"
"This test currently fails and we are working on fixing it"

The tests were disabled because they exposed real bugs (like #2018), but the bugs weren't fixed - the tests were just ignored instead.

Test Coverage Plan

Issue #2021 tracks fixing all ignored tests. The plan:

Phase 1: Triage - Run each ignored test, document failure modes
Phase 2: Fix Core Issues - Fix channel closed bugs, gateway startup issues, PUT timeouts
Phase 3: Un-ignore Tests - Re-enable tests as underlying bugs are fixed
Phase 4: Prevention - Enforce TODO-MUST-FIX for new ignored tests

This PR

This fix addresses one of the core issues preventing tests from working:

GET operations no longer fail when caching fails
This should unblock some of the ignored integration tests
Once test: fix all ignored integration tests #2021 work progresses, we can verify this fix with regression tests

Recommendation

Merge this PR to fix the immediate bug, then work on #2021 to restore proper test coverage.

@sanity - correct me if I've mischaracterized the testing situation.

[AI-assisted debugging and comment]

iduartgomez

I am not sure those tests specifically test this, I would recommend we add a test which specifically tests what is being fixed in this PR, so add that to the backlog.

Let's not block anyway.

sanity · 2025-10-29T14:39:33Z

@iduartgomez - Agreed! I'll create a backlog issue for a specific regression test.

The test should verify:

GET succeeds when local cache has identical state (no redundant PutQuery)
GET succeeds even when PutQuery fails (resilience layer)
Client receives data despite caching failures

I started working on an integration test but hit complexity with the test structure. The proper test should:

Use a contract that implements strict idempotency checks (like web-container-contract)
PUT the contract, then GET it twice
Verify second GET succeeds even though local state is identical

Will create a backlog issue tracking this specific test.

[AI-assisted debugging and comment]

sanity mentioned this pull request Oct 28, 2025

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

Closed

sanity requested a review from iduartgomez October 28, 2025 19:31

sanity changed the title ~~fix: GET operation should not fail when local caching fails~~ fix: make get operation resilient to local caching failures Oct 28, 2025

sanity requested a review from Copilot October 28, 2025 19:32

Copilot AI reviewed Oct 28, 2025

View reviewed changes

crates/core/src/operations/get.rs Outdated Show resolved Hide resolved

crates/core/src/operations/get.rs Outdated Show resolved Hide resolved

sanity force-pushed the fix/2018-get-idempotent-cache branch from 672da6f to 9cafbea Compare October 28, 2025 19:35

sanity force-pushed the fix/2018-get-idempotent-cache branch from 9cafbea to f3c17a3 Compare October 28, 2025 19:40

Merge branch 'main' into fix/2018-get-idempotent-cache

e9fe74e

iduartgomez reviewed Oct 29, 2025

View reviewed changes

iduartgomez approved these changes Oct 29, 2025

View reviewed changes

sanity mentioned this pull request Oct 29, 2025

test: add regression test for issue #2018 GET idempotent cache #2033

Open

4 tasks

sanity added this pull request to the merge queue Oct 29, 2025

Merged via the queue into main with commit 7929dfe Oct 29, 2025
12 checks passed

sanity deleted the fix/2018-get-idempotent-cache branch October 29, 2025 14:48

sanity mentioned this pull request Oct 29, 2025

PUT requests timeout in network mode - gateway never sends PutResponse #2035

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix: make get operation resilient to local caching failures #2020

fix: make get operation resilient to local caching failures #2020

Uh oh!

sanity commented Oct 28, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

iduartgomez left a comment

Uh oh!

sanity commented Oct 29, 2025 •

edited

Loading

Uh oh!

iduartgomez left a comment

Uh oh!

sanity commented Oct 29, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

fix: make get operation resilient to local caching failures #2020

fix: make get operation resilient to local caching failures #2020

Uh oh!

Conversation

sanity commented Oct 28, 2025

Summary

Problem

Solution

Layer 1: Prevention

Layer 2: Resilience

Changes

Testing

Why This Fix is Correct

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

iduartgomez left a comment

Choose a reason for hiding this comment

Uh oh!

sanity commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Regression Test Status

The Testing Problem

Why Tests Are Disabled

Test Coverage Plan

This PR

Recommendation

Uh oh!

iduartgomez left a comment

Choose a reason for hiding this comment

Uh oh!

sanity commented Oct 29, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sanity commented Oct 29, 2025 •

edited

Loading