feat: add X-Priority request header to the SDK#1167
Open
dgarros wants to merge 22 commits into
Open
Conversation
Phase 1 (Specify) of speckit prep: spec.md + requirements checklist for the SDK X-Priority feature — client-wide default + per-request override, zero behaviour change when unconfigured, async/sync parity. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Phase 2 (Plan): plan.md, research.md, data-model.md, quickstart.md, and API + wire contracts. Approach grounded in the existing X-Infrahub-Tracker prior art — default injected into base self.headers, per-request override applied in execute_graphql/_execute_graphql_with_file only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-259) Phase 3 (Critique): dual-lens (product + engineering) review — verdict PROCEED WITH UPDATES, no blockers. Applied low-risk recommendations: - SC-002 reworded (X-Priority absent vs literal byte-for-byte) - new SC-006 (batch/blob inherit the client default) - scoped the manual-header edge case to low-level _get/_post - plan/research notes: pagination forwards priority per page; multipart sets X-Priority after the content-type pop Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…259) Phase 4 (Tasks): 35 tasks organized by user story (US1 MVP → US5 parity) plus setup, foundational, and polish. Tests included per PRD testing decisions; critique follow-ups (pagination, multipart ordering, batch/blob inheritance) captured as explicit test tasks. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Phase 5: verdict ALIGNED against the IHS-259 PRD. All FRs, SCs, user stories, edge cases, and out-of-scope items carried over; only expansions (FR-009, SC-006) and one testability clarification (SC-002). 0 remediation passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add a case-insensitive Priority(str, Enum) in constants.py, re-export it from the SDK public namespace, and add a Config.priority field (auto-binding to INFRAHUB_PRIORITY, default None). Foundational prereqs T002-T004 for the X-Priority request header feature. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Inject the client-wide default priority once in BaseClient.__init__ so it rides GraphQL, multipart, and raw blob transports via the shared self.headers merge. Add unit tests covering GraphQL query/mutation, object-store blob download/upload, multipart upload, batched requests, and the always-emitted normal default, for both async and sync clients. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Lock backwards compatibility (US3): an unconfigured client emits no X-Priority header across GraphQL, multipart, and blob transports, and its baseline SDK headers (content-type, X-Infrahub-Tracker) are unchanged. Both async and sync clients. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…S-259] Add a `priority: Priority | None = None` keyword to the GraphQL execute funnels (execute_graphql, _execute_graphql_with_file) and thread it through the high-level client methods (get, all, filters, create_diff, get_diff_summary, get_diff_tree) and node methods (save, create, update, delete) on both the async and sync clients. Resolution is `per_request if per_request is not None else client_default`. Per-request headers now take precedence over self.headers in the transport helpers (_post/_get/_get_streaming/_post_multipart) so an explicit override is not clobbered by the client-wide default. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… unknown [IHS-259]
Lock the Priority enum + pydantic validation for Config.priority (US4):
- unknown value ("lowe") raises ValidationError at config load (no request)
- "LOW"/"Low"/"low"/Priority.LOW all resolve to Priority.LOW (and HIGH/NORMAL)
- INFRAHUB_PRIORITY env-var path resolves case-insensitively
- default Config() leaves priority None (header omitted)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…le [IHS-259] Add a data-driven parity test encoding the resolution truth table from data-model.md (8 default x override rows), run against both the async and sync clients (16 param cases) asserting identical emitted X-Priority headers. Audited T006-T012 and T021-T024: all already parametrized over ["standard","sync"], no change needed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… [IHS-259] Phase 8 polish for the X-Priority feature: - Add priority kwarg mention to get_diff_tree docstrings (async + sync) - Regenerate SDK docs (new Config.priority field in config.mdx; priority kwarg across client/node reference), also picking up pre-existing generator whitespace drift so docs-validate passes - Add changelog fragment 1151.added.md describing the Priority enum, Config.priority, and per-request priority= surface Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rge [IHS-259] The X-Priority feature flipped the transport-helper header merge so a per-request snapshot wins over the client base headers. For password-auth clients this regressed the automatic relogin retry: handle_relogin refreshes self.headers["Authorization"] to a new token, but the retry re-sent the SAME stale snapshot, which then overwrote the fresh token -> the retry used the expired token and auth failed. Introduce BaseClient._merge_request_headers, used at every transport-helper merge site (async/sync _post, _get, _get_streaming, _post_multipart). It lets per-request headers win (preserving the X-Priority override) but then re-asserts Authorization / X-INFRAHUB-KEY from self.headers so a mid-flight refreshed token always beats a stale snapshot. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… override [IHS-259] Add per-request X-Priority override tests for node.delete(), the save() update path on an existing node, and get_diff_summary()/get_diff_tree() (create_diff was already covered). All parametrized over async and sync clients. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Final report for the implement + review tail: 35/35 tasks done, 1 HIGH review finding (relogin auth-header regression) fixed inline, 3 test-gap findings closed. SDK suite 1145 passed; 8 pre-existing unrelated failures. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…HS-259] Re-run docs-generate on top of develop so the committed generated docs (config.mdx priority field, client/node method priority kwarg) match the current generator and markdown lint rules. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
the
type/documentation
Deploying infrahub-sdk-python with
|
| Latest commit: |
8b09904
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c73a8ead.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://dga-feat-x-priority-aa2nd.infrahub-sdk-python.pages.dev |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## develop #1167 +/- ##
===========================================
- Coverage 82.92% 82.78% -0.15%
===========================================
Files 139 139
Lines 12688 12210 -478
Branches 1906 1834 -72
===========================================
- Hits 10522 10108 -414
+ Misses 1598 1538 -60
+ Partials 568 564 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 6 files with indirect coverage changes 🚀 New features to boost your workflow:
|
rumdl (MD022/MD058) flagged missing blank lines around headings and tables in the auto-generated critique. Apply rumdl auto-fix so the markdown-lint CI job passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The file is gitignored on develop but was committed on this branch before that rule; untrack it while keeping the local working copy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review completed against the latest diff
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
… [IHS-259] Address review feedback: a per-request priority now rides consistently across a whole operation. - count() (both clients) accepts priority and forwards it; all(parallel=True) passes priority to its preliminary count query (previously sent at the client default while the pages used the override). - Resource-pool peer fetch after node create/update/save now inherits the operation's priority via _process_mutation_result -> RelatedNode.fetch. Tests cover count (direct + parallel all) and RelatedNode.fetch forwarding, both async and sync. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… [IHS-259] - Changelog: concise entry, no longer enumerates every method. - contracts/priority-api.md: remove the incorrect client.create(priority=) signature (it issues no request); document count/filters and the intentional exclusion; note peer-fetch inherits priority. - data-model.md: add node create() + resource-pool peer fetch to the coverage table; note client.create is n/a. - Regenerate SDK reference docs for the new count/fetch priority params. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… [IHS-259] Pass the deliberately-dynamic priority input through a dict[str, Any] so the runtime-coercion tests type-check cleanly, instead of suppressing the checker with an ignore comment. Behaviour and assertions unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ajtmccarty
reviewed
Jul 12, 2026
| "Config", | ||
| "InfrahubClient", | ||
| "InfrahubClientSync", | ||
| "Priority", |
Contributor
There was a problem hiding this comment.
I'd say, don't add this here. it makes it so that importing Priority requires importing Config and the clients
Comment on lines
+714
to
+715
| Args: | ||
| priority: Override the client-wide request priority for this query. When None, the client default is used. |
Contributor
There was a problem hiding this comment.
seems weird to document just 1 arg
| return InfrahubClient(config=self.config.clone(branch=branch)) | ||
|
|
||
| async def execute_graphql( | ||
| async def execute_graphql( # noqa: PLR0912 |
Contributor
There was a problem hiding this comment.
maybe this can be refactored to build the payload or headers or do the retry loop in a helper method to remove this typing ignore.
Comment on lines
+3425
to
+3427
| Args: | ||
| priority: Per-request priority emitted as the X-Priority header, overriding the client | ||
| default for this request only. When None, the client default (if any) is used. |
Comment on lines
+15
to
+17
| String-valued closed enum accepting values case-insensitively (e.g. "LOW", | ||
| "Low" and "low" all resolve to :attr:`Priority.LOW`). Unknown values raise | ||
| ``ValueError``, which surfaces as a ``pydantic.ValidationError`` at config load. |
Contributor
There was a problem hiding this comment.
"Case-insensitive" covers it. it's obviously a string, we know what case-insensitive means, raising a ValueError is standard enum behavior, mentioning the "config" load is not the responsibility of this class
|
|
||
| @pytest.mark.parametrize("client_type", client_types) | ||
| @pytest.mark.parametrize("case", [pytest.param(tc, id=tc.name) for tc in RESOLUTION_TRUTH_TABLE]) | ||
| async def test_resolution_truth_table_parity(case: ResolutionCase, client_type: str, httpx_mock: HTTPXMock) -> None: |
Contributor
There was a problem hiding this comment.
looks like this makes some of the execute_graphql tests above unnecessary, but I could be missing something
| if client_type == "standard": | ||
| result = await client.count(kind="CoreRepository", priority=Priority.HIGH) | ||
| else: | ||
| result = client.count(kind="CoreRepository", priority=Priority.HIGH) |
Contributor
There was a problem hiding this comment.
does this need to assert the priority on the requests or is match_headers in the httpx_mock enough?
| if isinstance(client, InfrahubClient): | ||
| await client.execute_graphql(query=query, branch_name="main") | ||
| else: | ||
| client.execute_graphql(query=query, branch_name="main") |
Contributor
There was a problem hiding this comment.
should this test include the priority override that seems to have semi-caused the issue? or the Authorization / X-INFRAHUB-KEY header?
| for auth_key in ("Authorization", "X-INFRAHUB-KEY"): | ||
| if auth_key in self.headers: | ||
| merged[auth_key] = self.headers[auth_key] | ||
| return merged |
Contributor
There was a problem hiding this comment.
I'm not entirely sure I understand how this issue might arise. the @handle_relogin calls Client.login, which sets self.headers["Authorization"]. so it seems like this would only be an issue if someone was including their auth information in the headers dict for every individual request. in which case, I would think that we should respect that b/c the user would be affirmatively forcing themself to handle relogin on their own. preventing them from setting a per-request override for these two keys feels wrong
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
2 participants
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.
Why
Infrahub's background systems (generators, artifacts, diffs, syncs, computed attributes) call back into the same API servers through this SDK, competing on equal footing with human/frontend traffic for a shared worker pool and database connections. Today the API layer cannot tell interactive traffic from background traffic, so under heavy background load it cannot preferentially protect the frontend.
Goal: let the originator of each request — the SDK caller — declare how important the request is, in a form the server can act on. This is the SDK's contribution to the server-side prioritization effort in INFP-636.
Non-goals: no server-side admission control, no 429/
Retry-Afterhandling (that is GitHub #1124), no traffic-classification guidance.Closes #1151
What changed
Behavioral changes (what callers can now do):
Config.priority(orINFRAHUB_PRIORITY, case-insensitive) and every request from that client carriesX-Priority: <value>across all transports — GraphQL, multipart upload, and raw blob_get/_post— with no call-site changes. Ideal for a client dedicated to background work (priority=Priority.LOW).priority=Priority.HIGHtoget,all,execute_graphql, the diff methods, and nodesave/create/update/deleteto override the default for exactly one request; the next un-annotated call reverts to the client default.Priorityenum (high/normal/low) is exported frominfrahub_sdk, so intent is expressed as a value rather than a raw header string; an invalid configured value is rejected at config-load time.X-Priorityheader is emitted — outgoing requests are unchanged from before this PR.Implementation notes:
X-INFRAHUB-KEYalready works). The per-request override is applied only in the two GraphQL funnels; higher-level methods forward the kwarg and never re-resolve. Resolution isper_request if per_request is not None else client_default.InfrahubClient(async) andInfrahubClientSync(sync) behave identically.What stayed the same:
Priorityenum,Config.priorityfield, and optionalpriority=kwarg. Generatedprotocols.pyuntouched.Authorization).Related context
Built with spec-kit; the spec, plan, critique, task breakdown, and implementation report live under
dev/specs/ihs-259-sdk-x-priority-header/. Server contract per INFP-636 (header is exactlyX-Priority; absent/unknown treated asnormalserver-side, which is why omitting the header when unset is a safe, incremental rollout). Complementary: GitHub #1124 (429 retry/backoff).How to review
Core change is small and funnels through a few points:
infrahub_sdk/constants.py— thePriorityenum + case-insensitive_missing_.infrahub_sdk/config.py— thepriorityfield.infrahub_sdk/client.py— base-header injection inBaseClient.__init__, thepriority=handling inexecute_graphql/_execute_graphql_with_file, and_merge_request_headers(per-request headers win, but live auth is re-asserted so relogin retries use the fresh token).infrahub_sdk/node/node.py—priority=forwarding onsave/create/update/delete.The
docs/.mdxchanges are generated (uv run invoke docs-generate).How to test
Expected: green across both
standard(async) andsyncclients. The suite asserts the header on the wire per transport, no header when unconfigured, per-request override + revert, explicitNORMALbeating aLOWdefault, config validation, the full resolution truth table, and the relogin-token-refresh regression. Full SDK suite (uv run pytest tests/unit/sdk/) is green (1145 passed).Impact & rollout
X-Priority). Safe to deploy independently of the server work.Config.priorityfield /INFRAHUB_PRIORITYenv var (default: unset).Checklist
changelog/1151.added.md)dev/specs/)Summary by cubic
Adds an
X-Priorityheader to tag SDK requests ashigh,normal, orlowfor server-side prioritization. Header is omitted when unset; async and sync clients behave the same.New Features
Priorityenum exported frominfrahub_sdk.Config.priorityorINFRAHUB_PRIORITY; applied to GraphQL, multipart uploads, blob_get/_post, and thecountquery used byall(parallel=True).priorityonget,all,count,execute_graphql,create_diff,get_diff_summary,get_diff_tree, and nodesave/create/update/delete; resource‑pool peer fetch after node mutations inherits the operation’s priority.X-Priority. No new dependencies.Bug Fixes
Authorizationwhen merging per-request headers.Written for commit 8b09904. Summary will update on new commits.