fix(executor): escalate UPDATE related-fetch to network when op_manager is wired#4006
Merged
Conversation
…er is wired Bridged-path fetch_related_for_validation was local-only and returned MissingRelated immediately on miss. Cross-node UPDATE flows broke because the receiving node hadn't yet cached the related contract — most visibly in freenet/mail#80 where bob's inbox UPDATE always carried RequestRelated for alice's AFT record. Escalate to network GET via start_sub_op_get when op_manager is Some; mock executors with op_manager=None preserve legacy MissingRelated behavior. Add test_update_missing_related_local_only_errors to pin the mock path. E2E validated against the iso-nodes harness.
Contributor
|
HEAD has the updated documentation. The Rule Review: Clean — one housekeeping noteRules checked: WarningsNone. Info
Checklist against rules:
Rule review against |
Address rule-review feedback on PR #4006: 1. Add test_update_missing_related_escalates_to_network_when_stubbed: extracts the network-escalation branch into a free fn fetch_related_via_network with a thread-local override (#[cfg(test)] only). The test wires a stub that records every requested ContractInstanceId, then asserts the related id was queried through the network branch — pre-fix this assertion would fail because the bridged path returned MissingRelated synchronously without consulting the network. 2. Update .claude/rules/contracts.md: the previous 'Network fetch during broadcast processing is deferred' note documented the local-only behavior that this PR replaces. Updated to reflect the local-first + network-escalation flow, with the rationale (cross-node first-time observers) and timeout bound.
1. .claude/rules/contracts.md step 3 in 'WHEN validate_state returns RequestRelated' updated to reflect local-then-network fallback. The previous text said 'Look up each locally (lookup_key + state_store.get)' which described the pre-fix behavior. 2. fetch_related_via_network: explain why _tx must stay bound (load-bearing for the receiver under future refactors that may return non-Copy guards).
Address rule-review on PR #4006: 1. test_update_missing_related_network_fetch_fails — covers SubOpGetOutcome::NotFound/Infra mapping in fetch_related_via_network. Stub returns Err(MissingRelated); test asserts the stub was invoked exactly once and the upsert surfaces the failure as an error. Pairs with the existing happy-path test. 2. Doc link fix: NETWORK_FETCH_OVERRIDE → TEST_NETWORK_FETCH_OVERRIDE in fetch_related_via_network rustdoc. 3. Trim the _tx binding comment to match what Transaction's Copy semantics actually guarantee. Previous wording suggested a drop-guard concern that isn't present today.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The bridged-path
fetch_related_for_validation(used by everyUpdateQueryupsert) was local-only:bridged_lookup_key+state_store.get, then immediatelyMissingRelatedon miss. Cross-node UPDATE flows blew up here whenever the receiving node hadn't yet cached the related contract.This is the root cause behind freenet/mail#80 — the recipient's inbox UPDATE always carried
RequestRelatedfor the sender's AFT record contract, the receiver had never seen that contract, the merge errored, and the broadcast had to bounce through ResyncRequest recovery instead of applying directly. The mail UI worked around it by inline-bundling related state viaRelatedStateAndDelta.Fix
The PUT path already had a network-aware sibling (
fetch_related_for_validation_network) that walkslocal_state_or_from_network. Rather than duplicate the dispatch, push the network-fallback logic into the bridged helper itself: try local first, then escalate via the samestart_sub_op_getdriver, gated onop_manager.is_some().Mock executors and local-only test harnesses have
op_manager == None, so the legacyMissingRelatedoutcome is preserved — pinned by the newtest_update_missing_related_local_only_errors.Test plan
cargo test -p freenet --lib pool_tests::related_contract_tests(13 tests, including new one) — all passDrive-by
crates/core/src/transport.rs:941had a pre-existinglet _ = ...that was tripping the workspacelet_underscore_must_useclippy lint and blocking the pre-commit hook. Renamed tolet _drain = ....