fix(https-outcalls): correct max_response_bytes semantics, cycle over-budgeting, and reject messages - #361
Conversation
…-budgeting, and reject messages Three corrections from #360, all verified against the interface spec, the `ic` mops package, and the replica source. max_response_bytes: the limit is not body-only. The spec measures it over header names and values plus the body, and it also bounds the transform's output (including Candid serialization overhead), so a transform cannot bring an oversized response under the cap. Adds the spec's header limits and notes they are enforced at argument-decode time on the request side. Cycles: "safe to over-budget" omitted that attached cycles are held for the duration of the call, so a hand-attached margin caps outcall concurrency. This is why both wrappers attach the exact computed amount. Reject messages: the troubleshooting block listed four paraphrases, two of which ("Body size exceeds limit", "Not enough cycles") are not real strings. Replaced with the exact messages and reject codes from the replica. Notably there are three distinct size-limit messages because response headers are counted before the body is read, and "Http body exceeds size limit of <N> bytes." prints the full cap rather than the remaining allowance, so it fires on bodies well under <N>. Also corrects the timeout pitfall: a timeout is a retryable SysTransient reject, not a trap. Closes #360
Skill Validation ReportValidating skill: /home/runner/work/icskills/icskills/skills/https-outcallsStructure
Frontmatter
Tokens
Markdown
Tokens
Content Analysis
Contamination Analysis
Result: 1 warning Project Checks |
… reject code Review feedback from @eichhorl on #361, all verified against dfinity/ic@339d220a83. The transform claim was too absolute. The cap is enforced twice: on the raw response as it arrives, and again on the transform's Candid-encoded output. Stripping headers cannot rescue a raw response that already exceeded the cap (that check runs first), but it can keep the transform's own output under it. Reworded the pitfall, the troubleshooting note, and the eval behaviour. There are two timeouts, not one: - remote server silent for 30s: SysFatal, "Timeout expired" (DEFAULT_HTTP_REQUEST_TIMEOUT_SECS = 30; tonic Code::Cancelled maps to SysFatal via grpc_status_code_to_reject) - subnet produces no response within 60s: SysTransient, "Canister http request timed out" (CANISTER_HTTP_TIMEOUT_INTERVAL = 60) Insufficient-cycles reject code is CanisterReject, not the ErrorCode name (ic-error-types: CanisterRejectedMessage => CanisterReject). Header-size error fires on strictly exceeding the cap, not on meeting it: checked_sub returns Some(0) when headers equal max_response_bytes. Eval case 3 splits the two transform facts into separate behaviours. The combined wording conflated them, and once corrected the case stopped discriminating (4/4 vs 4/4) because the baseline already knows the raw response is checked first. Split restores it: 5/5 with skill, 4/5 baseline, failing on the transform-output bound.
…t claim Review feedback from @eichhorl, mirroring the corrections on dfinity/icskills#361. The transform claim was too absolute. max_response_bytes is enforced twice: on the raw response as it arrives, and again on the transform's own output. Stripping headers in the transform cannot rescue a response that already exceeded the cap, since that check runs first, but it does keep the transform's own output within the cap. Also corrects the timeout pitfall, which claimed the call traps. There are two timeouts and neither traps: the remote server going silent for 30s rejects with SysFatal, and the subnet failing to produce a response within 60s rejects with SysTransient.
|
Thanks @eichhorl, all five verified against Transform nuance (SKILL.md:42, evaluations:28). You're right, and the correction is sharper than a wording tweak: the cap is enforced twice, on the raw response as it arrives and again on the transform's Candid-encoded output. Stripping headers cannot rescue a raw response that already exceeded the cap ( Worth flagging what this did to the eval. Splitting the claim apart mattered more than expected: with the two facts merged into one behavior, the corrected wording stopped discriminating entirely (4/4 with skill, 4/4 baseline) because the base model already knows the raw response is checked first. The transform-output bound was carrying all the signal. So case 3 now tests them as separate behaviors, and the delta is back: 5/5 with skill, 4/5 baseline, failing exactly on the second one. Two timeouts (SKILL.md:50). Confirmed both, including the reject codes:
The skill had conflated the two, attaching the 30s figure to the 60s message. Both are now documented separately in the pitfall and the troubleshooting block.
"met or exceeded" (SKILL.md:425). Correct, applied. The same two corrections are pushed to dfinity/developer-docs#352 (e27a82b), where the guide additionally claimed the call traps on timeout. |
…or the two timeouts Follow-up to the review, from reading the pricing and client code more closely (dfinity/ic@339d220a83). "Deadline Exceeded" [SysTransient] is a third timeout-shaped reject: the adapter did not answer the replica within its 60s deadline (client.rs:412-419). It is reachable under legacy pricing, since LegacyTracker always reports the full MAX_RESPONSE_TIME (pricing/src/legacy.rs:24-30), but rarer than the two already documented because the adapter's own 30s timeout usually fires first. Deliberately NOT added: "Insufficient cycles" [CanisterReject]. All four sites need either a PricingError from the budget tracker or a deadline below MAX_RESPONSE_TIME, and both are pay-as-you-go-only. PAYG is not selectable today (ALLOWED_HTTP_OUTCALLS_PRICING_VERSIONS = [PRICING_VERSION_LEGACY]), and under legacy the PAYG tracker runs only as a shadow inside DarkLaunchTracker, whose results feed a metric and never affect behaviour. A table headed "match on these" is the wrong place for a reject that cannot fire. Pitfall 5 spells out one implication the review left implicit: both checks compare against the SAME max_response_bytes, so a raw response that only just fits can still fail after the transform, because the Candid overhead is added on top. This is what makes stripping headers in the transform genuinely useful. New eval case for the two timeouts, which nothing covered: 4/4 with skill, 2/4 baseline (the baseline invents a single 2-minute end-to-end timeout and never names either reject code). Case 3, whose wording this sharpened, is unchanged at 5/5 with skill vs 4/5 baseline.
…e too
The review comment on the guide ("comments of dfinity/icskills#361 also apply
here") applies to concepts/https-outcalls.md as well, which this branch also
edits. Two bullets there still carried the claims the review corrected:
- The 2MB bullet said a transform "cannot bring an oversized response back
under the cap" full stop. The cap is enforced twice against the same value:
on the raw response in the adapter (rpc_server.rs:402-417, before the
transform runs) and on the transform's Candid-encoded output
(client.rs:250). Stripping headers cannot rescue a response that failed the
first check, but it does keep the transform's own output under the second.
- The timeout bullet described a single ~30s timeout. There are two: 30s for
the remote server (SysFatal, "Timeout expired") and 60s for the subnet to
produce a response (SysTransient, "Canister http request timed out").
Both now match the wording already applied to guides/backends/https-outcalls.mdx.
Verified against dfinity/ic@339d220a83.
|
One follow-up after your review, @eichhorl, pushed in 42bdb30. Two additions from reading the pricing and client code more closely. A third timeout-shaped reject. One I deliberately left out: Your transform point, one notch sharper. It had an implication the wording did not spell out: both checks compare against the same New eval case for the two timeouts, which nothing covered: 4/4 with skill, 2/4 baseline. The baseline invents a single fixed 2-minute end-to-end timeout and names neither reject code. Case 3, whose wording this sharpened, is unchanged at 5/5 vs 4/5. Both runs are in the PR body. The same concepts-page gap is fixed on dfinity/developer-docs#352. |
… default cost (#352) Closes #351. The issue reported two problems with how the HTTPS outcalls pages describe `max_response_bytes`. Both are confirmed against the [interface spec](https://github.com/dfinity/developer-docs/blob/main/docs/references/ic-interface-spec/management-canister.md) and fixed here, along with several further defects found while fixing them. ## What the issue reported **1. Wrong byte figure.** `2,097,152` → `2,000,000`. The spec: *"the default value of `2MB` (`2,000,000B`) is used as the limit."* Confirmed in the replica as `MAX_CANISTER_HTTP_RESPONSE_BYTES = 2_000_000`. **2. The limit is not body-scoped.** The spec defines the measured quantity as *"the total number of bytes representing the names and values of HTTP headers and the HTTP body."* Both pages now say headers plus body. **3. The transform bound** (raised in the issue body). `max_response_bytes` is enforced **twice**: on the raw response as it arrives, and again on the transform's output. A transform cannot rescue a response that already exceeded the cap, because the first check runs before the transform does; it only keeps the transform's own output within the cap. Stated in the guide's transform section, where a reader would form the "I'll strip headers to fit" plan, and in the concepts Limitations bullet. ## Additional defects found **4. The default-size cost was wrong on both pages.** Both said omitting `max_response_bytes` costs *~21.5 billion cycles*. The formula already published on `references/cycle-costs.md` gives: ``` 49_140_000 + 10_400 * 2_000_000 = 20_849_140_000 (~20.85 billion) ``` Corrected to ~20.85 billion in both places. 21.5B matches neither the decimal nor the binary reading, so it appears independently wrong rather than downstream of the byte-figure error. **5. `references/cycle-costs.md` said `max_response_bytes` defaults to "2 MiB".** Same decimal-vs-binary error, on the page the other two link to for exact pricing. Corrected, with the resulting cycle figure added. **6. Both pages claimed a single ~30 second timeout, and the guide said the call *traps*.** There are two timeouts and neither traps: | Trigger | Reject | Message | |---|---|---| | Remote server silent for 30s | `SysFatal` | `Timeout expired` | | Subnet produces no response within 60s | `SysTransient` | `Canister http request timed out` | Telling readers to expect a trap points them at the wrong error handling. **7. The Motoko cycle guidance was stale.** Both pages said *"In Motoko, cycles must be attached explicitly with `await (with cycles = ...)`"*. The `ic` package provides `Call.httpRequest`, which computes the exact cost via `ic0.cost_http_request` and attaches it, matching the Rust wrapper. The pages now also explain why a hand-picked margin is counterproductive: attached cycles are held for the duration of the call, so a margin caps outcall concurrency. ## Submodule bump Item 7 could not be fixed in prose alone, because the embedded Motoko snippets hardcoded `with cycles = 230_949_972_000`: correcting the text would have left the page contradicting its own code. That was fixed upstream first in dfinity/examples#1477, merged as `b4fe175`. `.sources/examples` is bumped `d4ea422` → `b4fe175` here, so the snippets now render `await Call.httpRequest(request)` and code and prose agree. The old pin predated the examples restructure, so all six `snippet=` paths moved and are updated: ``` send_http_{get,post}/src/send_http_{get,post}_backend/main.mo -> send_http_{get,post}/backend/main.mo send_http_{get,post}/src/send_http_{get,post}_backend/src/lib.rs -> send_http_{get,post}/backend/src/lib.rs ``` Region names (`transform`, `get_request`, `post_request`) are unchanged. Per `.agents/submodule-bumping.md`: `guides/backends/https-outcalls.mdx` is the only page using `CodeExample`, so no other page is affected by the moves, and `examples` tracks master so it carries no `.sources/VERSIONS` entry. ## Scope Kept deliberately tight per `CONTRIBUTING.md`: `concepts/` stays explanatory, and the spec's header limits (≤64 headers, ≤8 KiB per name or value, ≤48 KiB total) are **not** added. The issue marked them optional, and enumerating them duplicates content that belongs in the interface spec and the `https-outcalls` skill. ## Verification - `npm run validate`: no errors in the touched files. - `build_and_deploy`: passing against the new submodule. This is the meaningful check for the bump, since `remark-snippet` treats a missing file or region as a hard build error. - Before pushing the bump, all six file+region pairs were confirmed to resolve at `b4fe175` by replicating the plugin's extraction logic. ## Related - dfinity/icskills#361 carries the same corrections in the `https-outcalls` skill, including the reject-message set these pages do not enumerate. - #254 (flexible outcalls) will invalidate the v1 pricing assumptions on these pages when it lands: `max_response_bytes` is *ignored* under pricing v2, and `ic0.cost_http_request` is deprecated. Flagged there with the specific lines, including that `references/cycle-costs.md` needs both cost models rather than an edit in place. As of `dfinity/ic@339d220a83` v2 is still gated off, so the pages are correct today.
Closes #360.
All three points in the issue are valid. Verified independently against the interface spec (
developer-docs/docs/references/ic-interface-spec/management-canister.md), theicmops package (v4.2.0), and the replica source (dfinity/ic@339d220a83) rather than taking the issue at its word.1.
max_response_bytesis not body-only (pitfalls 4, 5, 6)The skill said "The maximum response body is 2MB". The spec measures the limit over header names and values plus the body, and the same definition caps the request you send. A real API commonly sends 1–2 KB of response headers before a single byte of body, which against a tight cap is a large share of the budget, and the failure is total rather than a truncation.
The spec also binds the transform's output to
max_response_bytes(including Candid serialization overhead), so a tight cap cannot be rescued by stripping headers in the transform. That is now its own pitfall, since it is the natural wrong inference from the old wording.Also adds the spec's header limits (≤64 headers, ≤8 KiB per name or value, ≤48 KiB combined, URL ≤8192) with the non-obvious part: on the request side these are enforced when the replica decodes your arguments, so an over-limit request never leaves the subnet and fails with
InvalidManagementPayload, not with anything HTTP-looking.2. Over-budgeting cycles is safe but not free (Cycle Cost Estimation)
"Unused cycles are refunded, so it is safe to over-budget" was true but misleading. Attached cycles leave the canister's spendable balance for the duration of the call, so a hand-attached margin caps how many outcalls can be in flight. For a canister making one outcall per user action that margin is a concurrency limit. This is exactly why both wrappers attach the computed amount:
Cost.httpRequestcallsPrim.costHttpRequest(requestSize, maxResponseBytes)with no margin, and the package documents the reason.3. Two of the four documented error strings were invented
Not in the issue. Found while fixing §2, and the more consequential defect, since agents match on these.
"Body size exceeds limit"and"Not enough cycles"do not exist in the replica. The real strings:Timeout expiredSysFatalCanister http request timed outSysTransientDeadline ExceededSysTransientNo consensus could be reached. Replicas had different responses. …SysTransientHeader size exceeds specified response size limit <N>SysFatalHttp body exceeds size limit of <N> bytes.SysFatalTransformed http response exceeds limit: <N>SysFatalhttp_request request sent with <X> cycles, but <Y> cycles are required.CanisterRejectThere is no single "response too large" error: headers are subtracted from
max_response_bytesbefore the body is read (rpc_server.rs:403), which is why there are three. And the body message interpolates the full cap rather than the remainder it actually measured against, so a 3 KB body can fail with "exceeds size limit of 10000 bytes". That is the issue's own §2 misreading, baked into the error text.Also corrects the timeout pitfall. The skill claimed a single ~30s timeout that traps. There are two, and neither traps: 30s for the remote server (
SysFatal,Timeout expired) and 60s for the subnet to produce a response (SysTransient,Canister http request timed out, the retryable one). The skill had attached the 30s figure to the 60s message.Evals
Four adversarial cases added. Cases 1 and 2 re-run because this PR rewrites content they cover. All six run with baseline.
max_response_bytescovers headers, bounds transform outputHttp body exceeds size limiton an under-cap bodyTwo things worth noting in these numbers:
Full eval output
Review follow-up
Two rounds after @eichhorl's review, both verified against
dfinity/ic@339d220a83.Round 1 (a180e9a) applied all five comments: the transform nuance (the cap is enforced twice, so stripping headers cannot rescue a raw response that already failed the first check but does bound the transform's own output), the two timeouts,
CanisterRejectin place of theErrorCodename, andexceededrather thanmet or exceededfor the header-size message (checked_subreturnsSome(0)on equality). Case 3 was split into two behaviours, because with the two facts merged the corrected wording stopped discriminating at all (4/4 vs 4/4): the base model already knows the raw response is checked first, and the transform-output bound was carrying the whole signal.Round 2 (42bdb30) adds
Deadline Exceeded[SysTransient]to the reject table: the adapter did not answer the replica within its 60s deadline (client/src/client.rs:412-419), reachable under legacy pricing sinceLegacyTrackeralways reports the fullMAX_RESPONSE_TIME(pricing/src/legacy.rs:24-30), though rarer than the two above because the adapter's own 30s timeout usually fires first. It also spells out one implication the review left implicit: both size checks compare against the samemax_response_bytes, which is why a raw response that only just fits can still fail after the transform, the Candid overhead being added on top. That is precisely when stripping headers in the transform buys real room.Insufficient cycles[CanisterReject](client.rs:304/414/436/580) was deliberately left out. Every site needs either aPricingErrorfrom the budget tracker or a deadline belowMAX_RESPONSE_TIME, and both are pay-as-you-go-only:ALLOWED_HTTP_OUTCALLS_PRICING_VERSIONS = &[PRICING_VERSION_LEGACY](management_canister_types/src/http.rs:72),LegacyTrackernever returnsPricingError, and under legacy the pay-as-you-go tracker runs only as a shadow insideDarkLaunchTracker, whose results feed a metric and never affect behaviour. A table headed "match on these" is the wrong home for a reject that cannot currently fire.Follow-up eval runs: new case 6, and case 3 re-run after the wording change
Note that the case-3 output inside the block above supersedes the case-3 section in the earlier block, which predates the split into two behaviours.
npm run validatepasses, warnings unchanged.Related
The issue's §3 (the docs page stating
2,097,152) is a developer-docs defect; the skill was already correct. Tracked and fixed separately:with cycles = 230_949_972_000toCall.httpRequest— the same over-budgeting anti-pattern as §1, shipped as the canonical example