Fix http.batch() drops valid requests when one entry fails to parse

[chojs23]

What?

• Let http.batch() keep preparing all entries even when some parse/validation steps fail.
• Accumulate valid requests, record per-entry errors, and only remember the earliest prep error for reporting.
• Update TestBatchErrorNoPanic to assert that valid entries still execute and succeed when malformed ones are present.

Why?

Previously, any malformed entry caused http.batch() to bail out before scheduling the rest, so one typo could silently drop an entire batch of valid requests. The new behavior keeps the old error reporting semantics (first prep error drives throw/log) while ensuring the other requests still run and their responses reflect what actually happened.

Checklist

• I have performed a self-review of my code.
• I have commented on my code, particularly in hard-to-understand areas.
• I have added tests for my changes.
• I have run linter and tests locally (make check) and all pass.

Checklist: Documentation (only for k6 maintainers and if relevant)

Please do not merge this PR until the following items are filled out.

• I have added the correct milestone and labels to the PR.
• I have updated the release notes: link
• I have updated or added an issue to the k6-documentation: grafana/k6-docs#NUMBER if applicable
• I have updated or added an issue to the TypeScript definitions: grafana/k6-DefinitelyTyped#NUMBER if applicable

Related PR(s)/Issue(s)

Closes #5497

[oleiade]

Hi @chojs23

Could you please provide examples of what the current behavior is, and what your expected behavior should be instead. I've been looking into the PR, but I've been having a hard time wrapping my head around the concrete use-cases and behavior changes we're addressing here.

Thank you! 🙇🏻

[chojs23]

Hi @oleiade

Current behavior is that http.batch stops parsing at the first invalid entry. No later requests are executed. With throw false it returns a results array of the same length but only the failed slot has a Response and the rest are null. With throw true it throws the parse error and you get no results.

With this example

	const res = http.batch({
		good: ["GET", "https://httpbin.test.k6.io/get"],
		good2: ["GET", "https://httpbin.test.k6.io/get"],
		bad: ["GET", "://missing-schema"],
		good3: ["GET", "https://httpbin.test.k6.io/get"],
	});

The entries that were visited before the error, like good and good2, get Response objects but their requests are not executed and show empty fields and status 0. The bad entry gets error and error_code 1020 because the URL is invalid. And there is no good3 entry

It is inconvenient when batch input is built from data or config and one bad URL and it would be better not block all other valid requests. Common cases are CSV or JSON driven tests, multi tenant targets where one tenant is misconfigured, and service checks where one bad endpoint should not stop checks for the rest.

[oleiade]

Hey @chojs23

Confirming that I could effectively reproduce the behaviour you mentioned. Thanks a lot for providing an illustrative example 🙇🏻

I'm not certain what the best way to go about this is, and need a bit more time to think about it. While your approach is sound, and the PR looks good at first glance, I'll take the time to make a bit of experimentation on my own and ask around in the maintainers' team what they think about it, and come back to you as soon as I have a clearer picture. Stay tuned 📻

[oleiade]

Having spent a bit more time playing around this PR and toying with the idea and potential consequences, I'm of the opinion that once reviewed we should most likely merge this.

The current behavior really does look like a bug, not a feature. If this was explicitly designed and intend to behave as such, it hasn't been documented, and thus, we can only assume this is indeed unintended.

Users can't reasonably depend on this behaviour, because as you've shown it's inconsistent — valid requests return "successful" responses with status 0 but never actually execute. That's misleading.

Breaking change

One concern I have, is that regardless of what the right behaviour should be, this changes can be considered a breaking change, in the odd chance that users out there did use a workaround along the lines of:

const responses = http.batch([...]);
if (responses.some(r => r.status === 0)) {
  // Assume batch failed, retry everything
}

Nonetheless I think this is a rather fragile workaround for the current broken behaviour, and not something we should preserve.

If we decide to go ahead and merge this indeed, we will need to do that for k6 v2.0.0, which is scheduled for end of april/beginning of may (no action required on your end @chojs23).

What do you think @ankur22 ?

[ankur22]

I think it depends on the use case as to what should happen. I've never used it myself, but after taking a quick look at the API and the docs, I would have thought that users would want it to fail fast and tell them what and where the invalid elements are in the input array.

I agree that it does look a bit odd that the result of a failed validation includes empty responses for valid input elements prior to the invalid element -- I would personally only want to see the list of problematic elements so that I can quickly resolve them.

Maybe there needs to be a failFast flag for this API:

    // Will validate all input elements, run only if all elements are valid,
    // if any elements in the array are invalid it will take note of them and
    // throw an exception after validating all elements.
    try {
        const res = http.batch({
            good: ["GET", "https://httpbin.test.k6.io/get"],
            good2: ["GET", "https://httpbin.test.k6.io/get"],
            bad: ["GET", "://missing-schema"],
            good3: ["GET", "https://httpbin.test.k6.io/get"],
        }, failFast: true);
    } catch (err) { ... }

    // Will try to execute all entries, ignore invalid entries, return all the
    // executed responses and validation errors, leaving it up to the user
    // to scan all the responses for valid/invalid responses.
    const res = http.batch({
        good: ["GET", "https://httpbin.test.k6.io/get"],
        good2: ["GET", "https://httpbin.test.k6.io/get"],
        bad: ["GET", "://missing-schema"],
        good3: ["GET", "https://httpbin.test.k6.io/get"],
    }, failFast: false);

I agree that whatever change we make will be a breaking change so we should avoid merging it until v2.0.0 👍

[ankur22]

Nit: This looks like a refactor, i think to make reviewing code a bit easier, refactors should go in a separate commit. Not to worry this time 🙂