Skip to content

fix(resources): fetch federated resource live when cached content is NULL - #5467

Closed
Ewertonslv wants to merge 1 commit into
IBM:mainfrom
Ewertonslv:fix/5450-cache-mode-federated-resource-read
Closed

fix(resources): fetch federated resource live when cached content is NULL#5467
Ewertonslv wants to merge 1 commit into
IBM:mainfrom
Ewertonslv:fix/5450-cache-mode-federated-resource-read

Conversation

@Ewertonslv

Copy link
Copy Markdown

🔗 Related Issue

Refs #5450 (scoped to the NULL-content case — see Scope below; not an auto-close).

📝 Summary

In cache gateway mode, ResourceService.read_resource reads
content = resource_db.content for a federated resource. When both
text_content and binary_content are NULL (e.g. resources created via the
migration / re-discovery path, createdVia: rediscovery), the Resource.content
property raises ValueError("Resource has no content") before the RESOLVE
CONTENT block. The transport layer catches the exception and returns empty
content, so the client silently receives an empty read instead of the upstream's
real content.

Fix

Catch that ValueError in the cache-mode branch and, when the resource is
federated (has a gateway), build a metadata-only ResourceContent so the
existing RESOLVE CONTENT block fetches it live from the gateway — the same
path already used for resources whose cached content is an empty string. A
non-federated resource has nothing to fetch, so the original error is preserved
(re-raised).

Tests (before → after)

  • ..._null_content_federated_fetches_live: NULL content + gateway → without the
    fix, ValueError propagates and the read is empty; with the fix, the content is
    fetched live via invoke_resource. (RED→GREEN verified locally with git stash.)
  • ..._null_content_no_gateway_preserved: NULL content + no gateway → the
    "no content" error is preserved and no live fetch is attempted.

pytest tests/unit/mcpgateway/services/test_resource_service.py passes locally
(full file green, including module doctests via --doctest-modules).

🔎 Scope / follow-up (binary resources)

The issue also notes that binary federated resources read empty for a
separate reason: invoke_resource's SSE/StreamableHTTP helpers extract the live
payload with getattr(contents[0], "text") unconditionally, so a
BlobResourceContents (e.g. image/png) yields None. Fixing that cleanly
requires invoke_resource to signal which field (text vs blob) the upstream
returned — both are strings, so they can't be disambiguated after the fact — i.e.
a small change to its return contract, plus routing in the RESOLVE block. That is
a deliberate design decision I left out of this PR to keep it focused and
low-risk. Happy to follow up with that in a separate PR if you'd like — just let
me know your preference on the invoke_resource return shape.

📏 Reviewability

  • This PR has one clear purpose
  • The linked issue is not labeled triage
  • Unrelated bugs or improvements are tracked in separate issues/PRs
  • Tests are included with the code they validate
  • If AI-assisted, I understand and can explain the generated changes

🏷️ Type of Change

  • Bug fix

…NULL

In cache gateway mode, read_resource reads `content = resource_db.content`
for a federated resource. When both text_content and binary_content are NULL
(e.g. resources created via the migration / re-discovery path), the
Resource.content property raises ValueError("Resource has no content") before
the RESOLVE CONTENT block. The transport layer catches it and returns empty
content, so the client silently receives an empty read instead of the
upstream's real content.

Catch that ValueError in the cache-mode branch and, when the resource is
federated (has a gateway), build a metadata-only ResourceContent so the
existing RESOLVE CONTENT block fetches it live from the gateway -- the same
path already used for resources whose cached content is an empty string.
Non-federated resources have nothing to fetch, so the original error is
preserved.

Adds regression tests for the NULL-content federated read (fetches live) and
the non-federated case (error preserved, no live fetch attempted).

Refs IBM#5450

Signed-off-by: Ewerton Silva <ewertoncom297@gmail.com>
@Ewertonslv

Copy link
Copy Markdown
Author

Correction to my checklist above: I ticked "the linked issue is not labeled triage", but #5450 is in fact still labeled triage — I misread the label state when opening this. Apologies. I've commented on the issue with the root-cause analysis so a maintainer can triage it, and I'm happy for this PR to sit until that happens, or to close it if the approach isn't wanted.

For coordination: this and #5463 were opened the same day and both touch read_resource in resource_service.py, but they fix different bugs on different code paths and their hunks don't overlap (lines ~2430 vs ~2417), so they merge cleanly in either order and neither depends on the other. Kept separate deliberately, per the one-concern rule. DCO is signed off.

@jonpspri jonpspri added the COULD P3: Nice-to-have features with minimal impact if left out; included if time permits label Aug 31, 2026
@jonpspri

Copy link
Copy Markdown
Collaborator

Hi @Ewertonslv — sincere apologies for leaving this PR open for so long without a proper review or merge. That's on us, and we're sorry for the lack of responsiveness.

We're closing this PR now as part of a housekeeping pass on long-open contributions.

If you're still interested in contributing this change, please feel free to:

  1. Open an issue describing the problem or feature this PR addresses (all PRs in this repository should be associated with a tracking issue)
  2. Rebase your branch on the current main
  3. Re-open this PR (or open a fresh one) with a reference to the new issue

We genuinely appreciate your contribution and hope to give it the attention it deserves. Thank you for taking the time to contribute to ContextForge! 🙏

@Ewertonslv

Copy link
Copy Markdown
Author

Thanks @jonpspri — no worries.

Rebased onto current main (ffcc0822, clean rebase) and re-verified. GitHub would not let me re-open this PR after the rebase (force-pushing the head branch makes a closed PR non-reopenable), so I took the "or open a fresh one" path: #6504.

On step 1 (tracking issue): #5450 already exists and is still open, so I did not file a duplicate — happy to open a fresh one if you'd rather.

Still reproducible on today's main: cache mode does a bare content = resource_db.content, so a federated row with NULL cached content raises ValueError: Resource has no content instead of proxying upstream. With the production hunk reverted, ..._null_content_federated_fetches_live fails with exactly that; with it applied, both new tests pass, the full test_resource_service.py is green (309 tests), and ruff/black are clean. Details in #6504.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

COULD P3: Nice-to-have features with minimal impact if left out; included if time permits

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG]: Federated resource read returns empty in default (cache) mode — NULL-content rows raise ValueError instead of proxying to upstream

2 participants