[Package Bug] mpkg loops forever fetching the listing when the index JSON has no `packages` key

[vadi2]

Package name: mpkg (v3.4)

Issue:

Brief summary

If the repository URL answers HTTP 200 with valid JSON that has no packages key, mpkg goes into an unguarded fetch loop and the Mudlet client becomes unusable. It never recovers, even after the repository starts serving a good listing again - the only way out is to kill Mudlet.

HTTP 404 and a non-JSON body are both handled cleanly (one "Failed to download package listing." and then the normal "Aborting, unable to read repository information." per command). So the missing guard is specifically the "valid JSON, wrong shape" case, which is what a proxy, a captive portal, or a future change to the listing format would produce.

Steps to reproduce

1. Point mpkg.repository at a server that returns {"name":"no packages key here"} for mpkg.packages.json.
2. Run mpkg update (or any command that calls mpkg.ready(), e.g. mpkg list).
3. The client starts hammering the repository and stops responding.

The loop

mpkg.ready()                -- mpkg.packages exists, mpkg.packages['packages'] is nil -> false
  -> mpkg.updatePackageList()
    -> downloadFile()
      -> sysDownloadDone
        -> mpkg.eventHandler()
          -> mpkg.checkForUpgrades(true)
            -> mpkg.ready(true)   -- still false
              -> mpkg.updatePackageList(true)  -- back to the top

Nothing breaks the cycle, and every command that calls ready() starts another concurrent chain, so the request rate grows rather than staying at one-in-flight.

Error output

Measured with a local mirror of the real listing (one file swapped for a packages-less body), Mudlet driven headlessly:

• ~23,000-26,000 mpkg.packages.json requests in ~3 minutes, on both clients tested
• console fills with Aborting, unable to read repository information. Retrying package listing update. / Updating package listing from repository. / Package listing downloaded. repeating forever
• both runs had to be SIGKILLed; switching the mirror back to a good listing did not stop the loop

Extra information

Reproduces identically on Mudlet 4.22.0 and current development (4.22.0-dev-b1fc20c19), so this is a package-side issue rather than a client regression. Found during a package backwards-compatibility sweep for the 5.0 release.

Related but not the same: #96 (suppressing repeated failure messages) - that suppresses the echo on the download-error path, whereas this loop runs entirely on the download-success path, so silentFailures never comes into play.

Fix ideas

• Treat "downloaded, but the body has no packages array" as a failure rather than a success: reject it in the sysDownloadDone handler before assigning mpkg.packages, and reuse the existing failure message + silentFailures handling.
• Guard mpkg.ready() so it cannot start a fetch while one is already in flight, and add a cool-down/backoff so a persistently bad listing retries on a timer instead of immediately.

[jaypatrickhoward]

Digging into this while working on #757, a couple of details that sharpen the mechanism.

The growth is geometric, because each completed download issues two new requests. In the sysDownloadDone branch, with a packages-less body:

1. mpkg.checkForUpgrades(true) → mpkg.ready(true) → false → updatePackageList(true) — one silent request.
2. The self-upgrade check immediately below evaluates mpkg.getRepositoryVersion("mpkg"), which begins with if not mpkg.ready() then return end — no argument, so silent is nil and it takes the if not silent branch → updatePackageList(false) — a second, explicit request.

So one completion spawns two, each of which spawns two. That fits the 23,000-26,000 requests in ~3 minutes better than a one-in, one-out loop would, and it's why the console interleaves Aborting, unable to read repository information. with Updating package listing from repository. — the second line comes from that ready() call inside the handler, not from anything the user typed.

The same no-argument pattern is the amplifier you describe for commands: mpkg.ready() is called with no argument at 8 sites (getRepositoryVersion, getDependencies, performUpgradeAll, install, upgrade, listInstalledPackages, search, show), so every one of them takes the explicit branch and starts a download rather than just failing.

Also worth noting getRepositoryVersion returns bare nil in that state, so the comparison reaches semver(nil). Worth checking what that does — if it raises, the handler aborts at that point every iteration, though both downloads have already been issued by then, so it wouldn't break the cycle.

The listing file itself isn't a factor. Checked on the Mudlet side: downloadFile writes via QSaveFile with a single write(reply->readAll()) after the transfer completes, so concurrent downloads to the same path can't interleave — the loser is replaced wholesale by a complete file. Mudlet also keys its downloadMap by QNetworkReply*, so each request gets its own event regardless. This is purely control flow; nothing is corrupting anything.

Two message defects share the same root and would be swept up by the same fix, since mpkg's bookkeeping is keyed on a single shared destination path:

• The sysDownloadError branch never clears displayUpdateMessage and returns early. So a failed explicit update leaves it armed, and the next successful refresh — hours later, from the timer — prints Package listing downloaded. unprompted.
• With two requests in flight and mixed outcomes, error-then-done prints Failed to download package listing. followed by Package listing downloaded.

On the fix shape, both of your ideas hold up, and there's a third that makes them exact. Because every request currently downloads to getMudletHomeDir() .. "/" .. mpkg.filename, the events can't be attributed to a caller. Giving each request its own saveto and keying a request table off it fixes that: sysDownloadDone carries the local filename as arg[1] and sysDownloadError as arg[2] (verified across all three raise sites in Mudlet), so attribution becomes exact, the in-flight count falls out of the table for free, and orphaned entries are identifiable for sweeping — which an opaque counter never is. The cost is temp-file naming plus cleanup on init.

For the ready() sites specifically, refusing a duplicate outright reads better than today's behaviour: those commands abort anyway, so the second download is pure waste, and an update is already in progress, please try again shortly is more accurate than Aborting, unable to read repository information. followed by a redundant fetch.

#757 deliberately doesn't touch any of this — it only stops the silent path writing shared state, so a background refresh can't suppress an explicit request's error message. The loop is untouched, and its reproduction should still match exactly.