fix(jp): address review regressions

src/nnnotes/apkset.py

@@ -1,29 +1,60 @@
 """Read a base APK and its splits, an APK directory, or an APKS/XAPK archive.
 
 The base manifest wins over split manifests. Resource members are resolved across the set;
-no merged APK is produced. Nested APKs are spooled so large asset packs need not stay in RAM.
+no merged APK is produced. Nested APKs are expanded once per archive revision to disk, with
+at most two idle APK sets retained per process; active readers keep their set alive until closed.
 """
 from __future__ import annotations
 
 import shutil
 import tempfile
+import threading
 import zipfile
 from contextlib import ExitStack
+from functools import lru_cache
 from pathlib import Path
 
+from .cache import file_id
+
+
+_EXPAND_LOCK = threading.Lock()
+
+
+@lru_cache(maxsize=2)
+def _expanded_apks(identity):
+    """An owned temporary directory and its APK paths; readers retain it across cache eviction."""
+    with zipfile.ZipFile(identity[0]) as outer:
+        names = [n for n in outer.namelist() if n.lower().endswith(".apk")]
+        bases = [n for n in names if Path(n).name == "base.apk"]
+        if len(bases) != 1:
+            raise ValueError("APK archive must contain exactly one base.apk")
+        directory = tempfile.TemporaryDirectory(prefix="nnnotes-apks-")
+        try:
+            paths = []
+            for i, name in enumerate(bases + sorted(n for n in names if n not in bases)):
+                path = Path(directory.name) / f"{i}.apk"
+                with outer.open(name) as stream, path.open("wb") as target:
+                    shutil.copyfileobj(stream, target)
+                paths.append(path)
+            return directory, paths
+        except BaseException:
+            directory.cleanup()
+            raise
+
 
 class ApkSet:
     def __init__(self, source):
         self.source = source
         self._stack = ExitStack()
         self._members = {}
+        self._expanded = None
 
     def __enter__(self):
         try:
             self._open()
             return self
         except BaseException:
-            self._stack.close()
+            self.__exit__(None, None, None)
             raise
 
     def _add(self, source):
@@ -42,18 +73,9 @@ def _open(self):
                 raise FileNotFoundError("APK directory has no base.apk")
             paths = [base] + sorted(p for p in path.glob("*.apk") if p != base)
         elif path.suffix.lower() in (".apks", ".xapk"):
-            outer = self._stack.enter_context(zipfile.ZipFile(path))
-            names = [n for n in outer.namelist() if n.lower().endswith(".apk")]
-            bases = [n for n in names if Path(n).name == "base.apk"]
-            if len(bases) != 1:
-                raise ValueError("APK archive must contain exactly one base.apk")
-            for name in bases + sorted(n for n in names if n not in bases):
-                tmp = self._stack.enter_context(tempfile.SpooledTemporaryFile(max_size=16 * 1024 * 1024))
-                with outer.open(name) as stream:
-                    shutil.copyfileobj(stream, tmp)
-                tmp.seek(0)
-                self._add(tmp)
-            return
+            with _EXPAND_LOCK:              # lru_cache alone permits duplicate concurrent expansions
+                self._expanded = _expanded_apks(file_id(path.resolve()))
+            paths = self._expanded[1]
         else:
             paths = [path]
             if path.name == "base.apk":
@@ -74,4 +96,8 @@ def read(self, name):
         return archive.read(info)
 
     def __exit__(self, *args):
-        return self._stack.__exit__(*args)
+        try:
+            return self._stack.__exit__(*args)
+        finally:
+            self._members.clear()
+            self._expanded = None

src/nnnotes/catalog.py

@@ -76,10 +76,11 @@ class Catalog:
     `cdn`: the region's CDN base (needed only to download what the cache lacks); `bundle_key`: the bundle
     decryption key (needed only for bundles not yet in the cache). Either may be a function that returns it, called
     the first time it is needed (a ConfigError it raises is raised naming the file that needed the setting).
+    `apk_catalog`: the stored catalog to replay without opening an APK; JP local reads verify it before extraction.
     """
 
     def __init__(self, catalog_bytes: bytes, cache_dir: Path, *, cdn=None, bundle_key=None, apk: Path | None = None,
-                 source=None, session=None):
+                 source=None, session=None, apk_catalog: bytes | None = None):
         self._settings = {"cdn": cdn, "bundle_key": bundle_key}
         self.cache_dir = Path(cache_dir)
         self.cache_dir.mkdir(parents=True, exist_ok=True)
@@ -88,10 +89,22 @@ def __init__(self, catalog_bytes: bytes, cache_dir: Path, *, cdn=None, bundle_ke
         self._sources = {"remote": catalog_bytes}
         self._locations: list[dict] | None = None
         self._parsed: tuple | None = None
-        if self.apk is not None:
+        if apk_catalog is not None:
+            self._sources["apk"] = apk_catalog
+        elif self.apk is not None:
             with ApkSet(self.apk) as z:
                 self._sources["apk"] = z.read(APK_CATALOG)
 
+    def check_apk(self, archive) -> None:
+        """Before extracting a JP local file, verify the APK against this catalog's snapshot."""
+        if self.source is not None and "apk" in self._sources:
+            try:
+                matches = archive.read(APK_CATALOG) == self._sources["apk"]
+            except KeyError:
+                matches = False
+            if not matches:
+                raise ConfigError("JP historical catalog needs the APK set it was imported with")
+
     def local_cache_dir(self) -> Path:
         """JP embedded files also depend on the APK catalog, independently of the CDN snapshot."""
         if self.source is not None and "apk" in self._sources:
@@ -278,6 +291,7 @@ def fetch(self, b: Bundle) -> Path:
                 raise apk_missing(f"bundle {b.name}")
             rel = b.internal_id[len(LOCAL_PREFIX):].lstrip("/")
             with ApkSet(self.apk) as z:
+                self.check_apk(z)
                 data = z.read(APK_AA_DIR + rel)
             key = self._setting("bundle_key", b.name) if data[:7] != b"UnityFS" else None
         _write_atomic(dst, _unityfs(data, b.name, key))
@@ -329,6 +343,7 @@ def apk_bundle(self, name_contains: str) -> Path:
             dst = self.local_cache_dir() / "bundles" / name
             if not (dst.exists() and dst.stat().st_size > 0):
                 dst.parent.mkdir(parents=True, exist_ok=True)
+                self.check_apk(z)
                 data = z.read(names[0])
                 key = self._setting("bundle_key", name) if data[:7] != b"UnityFS" else None
                 _write_atomic(dst, _unityfs(data, name, key))

src/nnnotes/cli.py

@@ -89,6 +89,8 @@ def load_config(args) -> Config:
     overrides = {k: getattr(args, dest, None) for k, (dest, _) in FLAG_SETTINGS.items()}
     cfg = Config.load(getattr(args, "config", None), overrides=overrides,
                       flags={k: flag for k, (_, flag) in FLAG_SETTINGS.items()})
+    if getattr(args, "func", None) is cmd_config_check:
+        return use(cfg)                         # diagnose raw settings before applying runtime defaults
     if cfg.provider() == "jp" and not cfg.has("catalog", "language"):
         cfg = cfg.for_region(cfg.region())
     return use(cfg)
@@ -617,7 +619,7 @@ def cmd_web(args, cfg):
                    if args.web_regions or args.all_regions else None)   # None: the one [catalog] region
         if (stories or models) and regions and len({cfg.provider(r) for r in regions}) > 1:
             raise ConfigError("build JP stories/models in a separate site directory from international releases")
-        if (stories or models) and regions and cfg.provider(regions[0]) == "jp":
+        if (stories or models) and regions:
             cfg = use(cfg.for_region(regions[0]))
         base = {"region": regions[0]} if regions else {}
         unknown = web.unknown_pairs(cfg, args.pair, regions) if args.pair else []

src/nnnotes/cli_assets.py

@@ -301,12 +301,10 @@ def catalog(self, vid: str):
                     session = Session(cfg, cfg.region())
                     cache = source.cache_dir(cache)
                 cat = Catalog(remote, cache, cdn=lambda: cfg.cdn(cfg.region()),
-                              bundle_key=lambda: _bundle_key(cfg), apk=cfg.path("paths", "apk"),
+                              bundle_key=lambda: _bundle_key(cfg), apk_catalog=apk,
                               source=source, session=session)
-                if source is not None and apk is not None and cat.sources().get("apk") != apk:
-                    raise ConfigError("JP historical catalog needs the APK set it was imported with")
-                # Replay the APK catalog imported with this version, not today's catalog offsets.
-                cat._sources = {"remote": remote, **({"apk": apk} if apk is not None else {})}
+                # Replay imported offsets; the current APK is needed only for uncached local files.
+                cat.apk = cfg.path("paths", "apk")
                 hit = self._catalogs[vid] = (cat, catalogdb.by_id(catalogdb.index(remote, apk)))
             return hit
 
@@ -321,23 +319,20 @@ def fetch_location(self, vid: str, lid: str) -> Path:
         if loc["kind"] == "bundle":
             return cat.fetch(Bundle(0, iid, file_name(iid), remote_path(iid) is not None))
         if remote_path(iid) is None:
-            return self._apk_file(iid, apk=cat.apk, cache=cat.local_cache_dir())
+            return self._apk_file(iid, cat)
         return cat.fetch_raw({"internal_id": iid})
 
-    def _apk_file(self, internal_id: str, *, apk=None, cache=None) -> Path:
-        """A raw file of the APK as stored, through the cache (raw/<its path below the APK's Addressables
-        directory>, where a CDN file of that path would be); KeyError when the APK does not hold it."""
+    def _apk_file(self, internal_id: str, cat) -> Path:
+        """A raw APK file through its snapshot's cache; only a cache miss needs the matching APK."""
         from .cache import write_atomic
         from .catalog import APK_AA_DIR
-        if self.cache is None:
-            raise self.cfg.missing("paths", "cache")
-        apk = apk or self.cfg.path("paths", "apk")
-        if apk is None:
-            raise self.cfg.missing("paths", "apk")
         rel = apk_rel(internal_id)
-        dst = (cache or self.cache) / "raw" / rel
+        dst = cat.local_cache_dir() / "raw" / rel
         if not (dst.is_file() and dst.stat().st_size > 0):
-            with ApkSet(apk) as z:
+            if cat.apk is None:
+                raise self.cfg.missing("paths", "apk")
+            with ApkSet(cat.apk) as z:
+                cat.check_apk(z)
                 data = z.read(APK_AA_DIR + rel)
             dst.parent.mkdir(parents=True, exist_ok=True)
             write_atomic(dst, data)

src/nnnotes/jp.py

@@ -160,8 +160,8 @@ def __init__(self, cfg, region):
     def observe(self, *, timeout=gameapi.TIMEOUT):
         with self._lock:
             section = f"servers.{self.region}"
-            api = gameapi.api_root(self.cfg, section)
-            origin(api)
+            api = gameapi.api_root(self.cfg, section).strip()
+            api = origin(api if "://" in api else "https://" + api)
             allowed = origin(self.cfg.cdn(self.region))
             client = gameapi.client_version(self.cfg, self.region)
             headers = {}

tests/test_apkset.py

@@ -0,0 +1,115 @@
+"""Small synthetic APK sets: expansion reuse, invalidation and reader lifetimes."""
+from concurrent.futures import ThreadPoolExecutor
+import os
+from pathlib import Path
+from unittest.mock import Mock
+
+import pytest
+
+from nnnotes import apkset
+from nnnotes.catalog import APK_AA_DIR, APK_CATALOG, Catalog
+from test_jp import cat_bytes, zip_bytes
+
+
+@pytest.fixture(autouse=True)
+def clear_expansions():
+    apkset._expanded_apks.cache_clear()
+    yield
+    apkset._expanded_apks.cache_clear()
+
+
+def package(path, payload=b"asset"):
+    path.write_bytes(zip_bytes({
+        "base.apk": zip_bytes({"AndroidManifest.xml": b"base"}),
+        "split_data.apk": zip_bytes({"asset": payload, "AndroidManifest.xml": b"split"}),
+    }))
+    return path
+
+
+@pytest.mark.parametrize("suffix", [".apks", ".xapk"])
+def test_repeated_and_concurrent_opens_expand_once(tmp_path, monkeypatch, suffix):
+    source = package(tmp_path / ("game" + suffix))
+    copy = Mock(wraps=apkset.shutil.copyfileobj)
+    monkeypatch.setattr(apkset.shutil, "copyfileobj", copy)
+    def read(_):
+        with apkset.ApkSet(source) as archive:
+            assert archive.read("AndroidManifest.xml") == b"base"
+            return archive.read("asset")
+    with ThreadPoolExecutor(4) as pool:
+        assert list(pool.map(read, range(8))) == [b"asset"] * 8
+    assert read(None) == b"asset"
+    assert copy.call_count == 2
+
+
+def test_replaced_archive_uses_new_expansion(tmp_path):
+    source = package(tmp_path / "game.apks")
+    with apkset.ApkSet(source) as old:
+        package(source, b"replacement")
+        with apkset.ApkSet(source) as new:
+            assert new.read("asset") == b"replacement"
+        assert old.read("asset") == b"asset"
+
+
+def test_same_size_archive_update_invalidates_by_mtime(tmp_path):
+    source = package(tmp_path / "game.apks", b"first")
+    before = source.stat()
+    with apkset.ApkSet(source) as archive:
+        assert archive.read("asset") == b"first"
+    package(source, b"other")
+    assert source.stat().st_size == before.st_size
+    os.utime(source, ns=(before.st_atime_ns, before.st_mtime_ns + 2_000_000_000))
+    with apkset.ApkSet(source) as archive:
+        assert archive.read("asset") == b"other"
+
+
+def test_eviction_does_not_remove_an_active_readers_files(tmp_path):
+    source = package(tmp_path / "first.apks")
+    with apkset.ApkSet(source) as first:
+        expanded = Path(first._expanded[0].name)
+        for name in ("second", "third"):
+            with apkset.ApkSet(package(tmp_path / f"{name}.apks")) as other:
+                assert other.read("asset") == b"asset"
+        assert apkset._expanded_apks.cache_info().currsize == 2
+        apkset._expanded_apks.cache_clear()
+        assert expanded.is_dir()
+        assert first.read("asset") == b"asset"
+    assert not expanded.exists()
+
+
+def test_failed_expansion_is_cleaned_and_can_be_retried(tmp_path, monkeypatch):
+    source = package(tmp_path / "game.apks")
+    directories = []
+    make_dir = apkset.tempfile.TemporaryDirectory
+    def temporary(**kwargs):
+        directory = make_dir(**kwargs)
+        directories.append(Path(directory.name))
+        return directory
+    monkeypatch.setattr(apkset.tempfile, "TemporaryDirectory", temporary)
+    copy = apkset.shutil.copyfileobj
+    def fail(stream, target):
+        copy(stream, target)
+        raise OSError("synthetic copy failure")
+    monkeypatch.setattr(apkset.shutil, "copyfileobj", fail)
+    with pytest.raises(OSError, match="synthetic copy failure"):
+        with apkset.ApkSet(source):
+            pytest.fail("partial expansion should not open")
+    assert directories and all(not p.exists() for p in directories)
+    assert apkset._expanded_apks.cache_info().currsize == 0
+    monkeypatch.setattr(apkset.shutil, "copyfileobj", copy)
+    with apkset.ApkSet(source) as archive:
+        assert archive.read("asset") == b"asset"
+
+
+def test_cached_apk_bundle_does_not_reexpand_the_archive(tmp_path, monkeypatch):
+    source = tmp_path / "game.apks"
+    source.write_bytes(zip_bytes({
+        "base.apk": zip_bytes({"AndroidManifest.xml": b"base"}),
+        "split_data.apk": zip_bytes({APK_CATALOG: cat_bytes(),
+                                     APK_AA_DIR + "Android/local.bundle": b"UnityFS\0local"}),
+    }))
+    cat = Catalog(cat_bytes(), tmp_path / "cache", apk=source)
+    copy = Mock(wraps=apkset.shutil.copyfileobj)
+    monkeypatch.setattr(apkset.shutil, "copyfileobj", copy)
+    assert cat.apk_bundle("local").read_bytes() == b"UnityFS\0local"
+    assert cat.apk_bundle("local").read_bytes() == b"UnityFS\0local"
+    assert copy.call_count == 0

tests/test_configfile.py

@@ -180,6 +180,30 @@ def test_edit_refuses_what_it_cannot_edit():
         configfile.edit('[paths]\nfonts.ja = "a"\n', [("paths.fonts", "ja", "b")], "f")
 
 
+@pytest.mark.parametrize("provider", ['"jpp"', '42'])
+def test_check_reports_invalid_provider_as_json(tmp_path, capsys, provider):
+    conf = tmp_path / "nnnotes.toml"
+    conf.write_text(f'[catalog]\nregion="jp"\n[servers.jp]\nprovider={provider}\n', encoding="utf-8")
+    code, out, err = run(["--config", str(conf), "config", "check", "--json"], capsys)
+    assert code == 1 and not err
+    report = json.loads(out)
+    assert report["problems"] == 1
+    item = next(s for s in report["settings"] if s["name"] == "servers.jp.provider")
+    assert item["status"] == "invalid"
+
+
+def test_check_does_not_turn_jp_language_defaults_into_flags(tmp_path, capsys):
+    conf = tmp_path / "nnnotes.toml"
+    conf.write_text('[catalog]\nregion="jp"\n[servers.jp]\nprovider="jp"\n', encoding="utf-8")
+    code, out, _ = run(["--config", str(conf), "config", "check", "--json"], capsys)
+    assert code == 0
+    settings = {s["name"]: s for s in json.loads(out)["settings"]}
+    assert settings["catalog.region"]["origin"] == "file"
+    assert settings["catalog.language"]["status"] == "unset"
+    cfg = cli.load_config(cli.build_parser().parse_args(["--config", str(conf), "master", "version"]))
+    assert cfg.require("catalog", "language") == "ja"  # normal commands still apply the JP default
+
+
 def test_check_reports_states_not_values(tmp_path, capsys, monkeypatch):
     (tmp_path / "nnnotes.toml").write_text(
         f'[bundle]\nkey = "{KEY}"\n[master]\niv = "00"\n[catalog]\nregion = "xx"\n[servers.tw]\ncdn = "ftp://h"\n'