fix(sources): never end a run early and call it Completed (ADR D72) - #264
Merged
Conversation
The runner's stuck-cursor guard (#263) bounds a source that makes no progress. This is the opposite failure direction, which the §6 audit found in four places: a source that stops early and reports the run Completed with rows missing. Nothing downstream can tell that apart from a genuinely complete run — no exception, no partial capture, no warning — which makes it the worst outcome the pipeline can produce. - Elasticsearch answers a partial search with HTTP 200: it sets `timed_out`, or reports failed shards under `_shards`, and returns what the responsive shards had. Neither field was inspected, so the short page simply ended pagination. Both are now checked before the hits are read, matching what GraphQL (D63) does with a 200 carrying `errors` and this source's own "full page with no sort values" guard. - `records.Count == pageSize` is no longer how "is there more?" is decided in OData's `Skip` and the HTTP source's `Page`/`Offset`. Inferring it from a full page is wrong whenever the service caps the page below what was requested — Dynamics, SAP Gateway and Business Central all clamp, and many REST APIs silently reduce an over-max limit, so against any of them the FIRST page comes back short and the run stopped there. They now page until a response comes back empty: one extra request per run, and this class of truncation becomes structurally impossible. NextLink and the cursor strategies are unaffected, since they follow a real token. - HubSpot and Airtable clamp the page size to the 100 their APIs accept rather than sending the engine's 1000 default and failing the first request (maintainer decision, D72). Safe because both derive HasMore from the server's own continuation token, so clamping only means more requests — had they inferred it from a full page, clamping alone would have been unsafe, which is exactly the bug above. Also clears CodeQL cs/linq/missed-select (alert 292), opened on master by the Link-header parsing in #262: the loop guards its split on the way in instead of assigning then checking, which is what read as a missed `.Select`. Eight tests, each verified to fail against the unfixed code. Three existing tests encoded the old full-page semantics and are updated rather than deleted; a new one covers the actual defect — a service that caps the page no longer truncates the report.
|
|
thiagoluga
added a commit
that referenced
this pull request
Aug 5, 2026
…eries they are CodeQL flagged two loops that filter their sequence implicitly (cs/linq/missed-where, alerts 293 and 294). Both are mine: 294 is this PR's parameter check, and 293 is the Link-header loop on master — #264 cleared cs/linq/missed-select there and CodeQL immediately raised missed-where on the same loop, so the guard-on-the-way-in shape traded one alert for another rather than fixing the cause. Splitting the parse of a single RFC 8288 link-value into its own method leaves ParseLinkHeaderNext reading as what it actually is — the first rel="next" target, if any — and removes the whole class of finding instead of moving it. The parameter check becomes the FirstOrDefault it always was; a default KeyValuePair has a null Key, which is precisely the "no complex parameter" answer. Behaviour is unchanged: Http 60/60, AspNetCore 159/159. The explanatory comment moved from between `=>` and the expression into the XML doc — dotnet format rejects a comment in that position, and it belongs with the rest of the method's documentation anyway.
thiagoluga
added a commit
that referenced
this pull request
Aug 5, 2026
…265) * fix(api): refuse structured run parameters at the boundary (ADR D72) Array/object parameter values were documented out of scope for v1, but nothing rejected them, and what happened next depended on which backend ran the job: the sync and in-memory paths handed the source a JsonElement — the very type an ADO provider cannot bind — while Hangfire round-tripped the bag and handed over raw JSON text. Either way the caller learned about the limit as a driver error partway through a run, attributed to the source rather than to the request that caused it. POST /reports/{name}/run now answers 400 naming the offending parameter, so the documented limit is real and identical on every backend, and the failure lands where the caller can act on it. Scalars are untouched — null explicitly still binds, since an optional parameter is an ordinary thing to send. Deliberately not applied to source property bags, which travel the same object?-valued shape: those are a provider's own configuration surface rather than a value bound into a query, and nothing in the audit showed them failing this way. Five tests, three of them verified to fail against the unfixed code. Both run modes are covered because the divergence between them was the defect, and the null case pins that the guard did not over-reach. This closes the last item of docs/STATUS-AND-BACKLOG.md §6 that did not need a maintainer decision. * refactor: express both Link parsing and the parameter check as the queries they are CodeQL flagged two loops that filter their sequence implicitly (cs/linq/missed-where, alerts 293 and 294). Both are mine: 294 is this PR's parameter check, and 293 is the Link-header loop on master — #264 cleared cs/linq/missed-select there and CodeQL immediately raised missed-where on the same loop, so the guard-on-the-way-in shape traded one alert for another rather than fixing the cause. Splitting the parse of a single RFC 8288 link-value into its own method leaves ParseLinkHeaderNext reading as what it actually is — the first rel="next" target, if any — and removes the whole class of finding instead of moving it. The parameter check becomes the FirstOrDefault it always was; a default KeyValuePair has a null Key, which is precisely the "no complex parameter" answer. Behaviour is unchanged: Http 60/60, AspNetCore 159/159. The explanatory comment moved from between `=>` and the expression into the XML doc — dotnet format rejects a comment in that position, and it belongs with the rest of the method's documentation anyway.
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.
The counterpart to #263. That PR bounded a source that makes no progress; this one covers the opposite failure direction the §6 audit found in four places: a source that stops early and reports the run as
Completedwith rows missing.That is the worst outcome the pipeline can produce. There's no exception, no partial-artifact capture, no warning — nothing downstream can distinguish it from a genuinely complete run. All four are resolved the same way: fail, or keep going, but never silently deliver less than was asked for. Recorded as the second half of ADR D72.
Elasticsearch partial searches now fail
A partial search is HTTP 200 with fewer hits than the shards hold: the cluster sets
timed_out, or reports failed shards under_shards, and returns what the responsive shards had. Neither field was inspected, so the short page just ended pagination.This matches what GraphQL (D63) already does with a 200 carrying
errors, and this source's own "full page with no sort values" guard. It does convert a previously-silent success into a hard failure — that's the intent; a report missing an unknown number of rows isn't a success.records.Count == pageSizeis no longer how "is there more?" is decidedOData's
Skipand the HTTP source'sPage/Offsethave no server token to follow, so it can only be inferred — but inferring from a full page is wrong whenever the service caps the page below what was requested. Dynamics, SAP Gateway and Business Central all clamp$top/limit, and many REST APIs silently reduce an over-max value. Against any of them the first page comes back short and the run stopped right there.They now page until a response comes back empty. Cost: one extra request at the end of a run. Benefit: this class of truncation is structurally impossible rather than merely unlikely.
NextLinkand the cursor strategies are untouched — they follow a real token.HubSpot and Airtable clamp instead of failing
Both cap at 100 while the engine defaults to 1000, so a source built with defaults failed its first request until the author happened to call
.PageSize(100)— a default configuration that could not work. Maintainer decision: clamp. An author shouldn't need to know each provider's ceiling.Worth noting why this is safe: both derive
HasMorefrom the server's own continuation token, so clamping only means more requests. Had they inferred it from a full page, clamping alone would have silently truncated — which is precisely the bug fixed in the section above. I checked this before applying the clamp rather than after.Also: CodeQL alert 292
cs/linq/missed-select, opened on master by my own Link-header parsing in #262. The loop now guards its split on the way in (if (link.Split(';') is not { Length: >= 2 } parts)) instead of assigning then checking, which is what read as a missed.Select. Rewriting it as an actual.Selectisn't possible — the loop has two guards and an early return.Tests
Eight, each verified to fail against the unfixed code:
Three existing tests encoded the old full-page semantics (
Page_strategy_pages_until_a_shorter_than_full_responseand friends) — updated to the new contract rather than deleted, and renamed so the name states what actually ends a run. One new test covers the real defect directly: a service that caps the page at 2 while 10 was requested no longer truncates the report. A third Elasticsearch test pins the negative case — a healthy response carrying"failed": 0must still pass, since the guard keys on> 0.No regressions across the family: Http 60, OData 51, Elasticsearch 55, HubSpot 16, Airtable 16, GraphQl 25, Salesforce 26, GoogleSheets 24, Core 303 — 0 failures.