fix: Deflater dictionary data loss, PATH lookup and other review findings, and narrower PR test runs

.github/pull_request_template.md

@@ -0,0 +1,11 @@
+## What this changes
+
+<!-- What the change does and why, in a few sentences. List the behavior changes. -->
+
+## Tests
+
+<!-- What was run: unit tests, programs compared with HotSpot, OpenJDK tests added to passing.txt. For performance work, the numbers (median and spread, master against branch). -->
+
+## Noticed, not fixed
+
+<!-- Problems found along the way that this PR leaves alone. Delete this section if there are none. -->

AGENTS.md

@@ -75,6 +75,13 @@ Node, which is how the tests run.
 - `project516-review-bot` reviews every PR. Merge only once its review of the
   head commit has every finding fixed or answered on its thread. When it
   cannot read the whole diff, a subagent reviews the files it skipped
+- Commit messages and PR titles follow Conventional Commits:
+  `type(scope): summary`, one sentence. Types: `feat`, `fix`, `perf`,
+  `refactor`, `test`, `docs`, `ci`, `build`, `chore`. The scope is optional
+  and names the area (`jit`, `net`, `fs`, `zip`, `jtreg`). Mark a change that
+  breaks the page API or the package layout with `!` (`feat!: ...`). PRs merge
+  by squash, so the PR title becomes the commit on `master`. PR bodies follow
+  `.github/pull_request_template.md`
 
 ## Glossary
 

docs/development.md

@@ -69,9 +69,14 @@ HotSpot with default options (for example `vm.continuations` is true and
 `vm.debug` is false); a test that needs more is skipped.
 
 `test/jtreg/passing.txt` lists the tests CI runs, with `--only-passing`.
-`--shard k/n` splits them into n shards of about equal run time, using each
-test's time on java-web from `test/jtreg/times.tsv` (the median for a test
-not in it). Each CI shard uploads its `build/jtreg/times.tsv` as an artifact.
+A pull request runs the tests its changes need (`scripts/jtreg/select.ts`): a
+change to a file the tests load runs the test directories the table there maps
+it to plus every tenth listed test, while a change to any other loaded file or
+to a workflow runs them all. Tests added to `passing.txt` or `locale-data.txt`
+run alone, and a change to `times.tsv` alone runs nothing. `--shard k/n` splits
+the selected tests into n shards of about equal run time, using each test's
+time on java-web from `test/jtreg/times.tsv` (the median for a test not in it).
+Each CI shard uploads its `build/jtreg/times.tsv` as an artifact.
 To refresh the file from a run, download those artifacts and merge them in:
 a test's new time replaces its old one, and tests the run left out keep theirs.
 

docs/support.md

@@ -28,7 +28,10 @@ java-web targets headless Java: console apps, libraries and javac.
   fails with "Function not implemented". Files on host file systems have the
   host's inode numbers and link counts, and the modes `open` and `mkdir` ask
   for, less the process's umask. In-memory files get synthetic inode numbers, a
-  link count of 1 and default modes.
+  link count of 1 and default modes. A `SecureDirectoryStream` works on its
+  directory's path, so if another process moves the directory and puts a new
+  one at that path, it acts on the new one; on HotSpot it keeps acting on the
+  directory it opened.
 - `-XX:OnOutOfMemoryError` runs its commands under Node only, and `StackWalker`
   has no live locals or operands. A virtual thread unmounts when it parks,
   blocks on a monitor or waits in `Object.wait`, holding its monitors, as in

scripts/jtreg/run.ts

@@ -448,22 +448,22 @@ async function javacInto(tc: TestCase, cp: string[], opts: string[], files: stri
   return rc === "0" ? null : msg;
 }
 
-function props(tc: TestCase): string[] {
+// the test properties jtreg sets for a test and expands in @run words (test.main.class is set at parse)
+function variables(tc: TestCase): Record<string, string> {
   const dest = classesDir(tc);
-  const srcPath = [tc.dir, ...tc.libraries.map((l) => libraryDir(tc, l))].join(":");
-  return [
-    `-Dtest.src=${tc.dir}`, `-Dtest.src.path=${srcPath}`, `-Dtest.classes=${dest}`, `-Dtest.class.path=${dest}`,
-    `-Dtest.jdk=${javaHome}`, `-Dcompile.jdk=${javaHome}`, `-Dtest.root=${testRoot}`, "-Dtest.vm.opts=", "-Dtest.java.opts=",
-    `-Dtest.file=${tc.file}`, `-Dtest.name=${tc.id}`,
-  ];
+  return {
+    "test.src": tc.dir, "test.src.path": [tc.dir, ...tc.libraries.map((l) => libraryDir(tc, l))].join(":"),
+    "test.classes": dest, "test.class.path": dest, "test.jdk": javaHome, "compile.jdk": javaHome, "test.root": testRoot,
+    "test.file": tc.file, "test.name": tc.id,
+  };
+}
+
+function props(tc: TestCase): string[] {
+  return [...Object.entries(variables(tc)).map(([k, v]) => `-D${k}=${v}`), "-Dtest.vm.opts=", "-Dtest.java.opts="];
 }
 
-// jtreg's ${...} in @run words (${test.main.class} is already in place)
 function expand(tc: TestCase, word: string): string {
-  const dest = classesDir(tc);
-  const values: Record<string, string> = {
-    "test.src": tc.dir, "test.classes": dest, "test.jdk": javaHome, "compile.jdk": javaHome, "test.root": testRoot,
-  };
+  const values = variables(tc);
   return word.replace(/\$\{([\w.]+)\}/g, (all, name) => values[name] ?? all);
 }
 
@@ -523,7 +523,7 @@ async function runCase(tc: TestCase): Promise<Record<string, unknown>> {
   const preview = tc.preview ? ["--enable-preview"] : [];
   // resource files next to a test; a package root above the test dir holds other tests' META-INF
   const cp = ["-cp", [...path, ...(packageRoot(tc) === tc.dir ? [tc.dir] : [])].join(":")];
-  const entry = (a: Action) => a.kind === "main" ? [a.main] : ["TestRunner", a.kind, a.main];
+  const entry = (a: Action) => a.kind === "main" ? [expand(tc, a.main)] : ["TestRunner", a.kind, expand(tc, a.main)];
   const words = (a: Action) => [...a.opts, ...props(tc)].map((w) => expand(tc, w));
   const argsOf = (a: Action) => a.args.map((w) => expand(tc, w));
   // the whole sequence on one VM, then the other: later runs may depend on files earlier ones left

scripts/jtreg/select.ts

@@ -4,15 +4,19 @@
 //
 // Prints GitHub Actions outputs: `filters`, the tests to pass to run.ts (empty
 // for all of them), and `shards`, a JSON list of shard numbers ([] when no test
-// is needed). Tests added to passing.txt run alone; a change to anything the
-// tests load runs them all.
+// is needed). Tests added to passing.txt or locale-data.txt run alone. A change
+// to a loaded file with a table entry runs the test directories it maps to,
+// plus every tenth listed test; a change to any other loaded file runs them
+// all. A change to times.tsv alone runs none.
 
 import { execFileSync } from "node:child_process";
 import { readFileSync } from "node:fs";
 import { dirname, join } from "node:path";
 
 const root = join(import.meta.dirname, "..", "..");
 const passingFile = "test/jtreg/passing.txt";
+const localeFile = "test/jtreg/locale-data.txt";
+const timesFile = "test/jtreg/times.tsv";
 
 // TypeScript the tests load: everything src/node.ts (the VM each test runs on),
 // the web build that bundles it into dist/node.js, the runner and this
@@ -34,6 +38,39 @@ function loaded(): Set<string> {
 }
 const notLoaded = /\.md$|^docs\/|^LICENSE|^renovate\.json$|^\.coderabbit|^types\/|^test\/(?!jtreg\/)/;
 
+// The test directories a change to a loaded file can affect. A file not here
+// runs every test, so only add one when its effect is known and narrow.
+const zipTests = ["java/util/zip", "java/util/jar", "jdk/nio/zipfs", "java/net/URLClassLoader"];
+const netTests = ["java/net", "java/nio/channels", "javax/net", "sun/net"];
+const ioTests = ["java/io", "java/nio", ...zipTests, "java/util/prefs",
+  "java/lang/ProcessBuilder", "java/lang/Process", "java/lang/RuntimeTests"];
+const processTests = ["java/lang/ProcessBuilder", "java/lang/Process", "java/lang/ProcessHandle", "java/lang/RuntimeTests"];
+const awtTests = ["java/awt", "java/beans", "javax/imageio"];
+const cryptoTests = ["java/security", "javax/crypto", "javax/net", "java/math", "sun/security"];
+const managementTests = ["java/lang/management", "java/util/concurrent"];
+
+const affects: [string, string[]][] = [
+  ["src/natives/zip.ts", zipTests],
+  ["src/inflate.ts", zipTests],
+  ["src/deflate.ts", zipTests],
+  ["src/natives/net.ts", netTests],
+  ["src/sockets.ts", netTests],
+  ["src/net-node.ts", netTests],
+  ["src/natives/sockets.ts", netTests],
+  ["src/http-proxy.ts", netTests],
+  ["src/natives/nio.ts", ioTests],
+  ["src/natives/io.ts", ioTests],
+  ["src/fs.ts", ioTests],
+  ["src/process-node.ts", processTests],
+  ["src/natives/java2d*.ts", awtTests],
+  ["src/natives/font*.ts", awtTests],
+  ["src/natives/crypto.ts", cryptoTests],
+  ["src/natives/management.ts", managementTests],
+];
+const patterns = affects.map(([file, dirs]) =>
+  [new RegExp(`^${file.replace(/[.+^${}()|[\]\\]/g, "\\$&").replaceAll("*", ".*")}$`), dirs] as const);
+const dirsFor = (file: string) => patterns.find(([re]) => re.test(file))?.[1];
+
 const shardsFor = (n: number) => Array.from({ length: Math.min(8, Math.ceil(n / 400)) }, (_, i) => i + 1);
 const listed = readFileSync(join(root, passingFile), "utf8").split("\n").map((l) => l.trim()).filter((l) => l && !l.startsWith("#"));
 const under = (id: string, f: string) => id === f || id.startsWith(f + "/") || id.startsWith(f + "#");
@@ -43,16 +80,29 @@ function select(base: string | undefined): string[] | "all" {
   const git = (...args: string[]) => execFileSync("git", args, { cwd: root, encoding: "utf8" });
   const filters = new Set<string>();
   let graph: Set<string> | undefined;
+  let dirs: string[] | undefined;
   for (const file of git("diff", "--name-only", "--no-renames", `${base}...HEAD`).split("\n").filter(Boolean)) {
-    if (file === passingFile) {
-      // the tests it adds
+    if (file === passingFile || file === localeFile) {
+      // the tests it adds, and listed tests that leave locale-data.txt and so run differently
       for (const l of git("diff", "-U0", `${base}...HEAD`, "--", file).split("\n")) {
-        if (l.startsWith("+") && !l.startsWith("+++") && !l.startsWith("+#") && l.slice(1).trim()) filters.add(l.slice(1).trim());
+        const id = l.slice(1).trim();
+        if (!id || id.startsWith("#") || l.startsWith("+++") || l.startsWith("---")) continue;
+        if (l.startsWith("+") || (l.startsWith("-") && file === localeFile && listed.includes(id))) filters.add(id);
       }
       continue;
     }
-    const ts = /^(src|scripts)\/.*\.ts$/.test(file);
-    if (ts ? (graph ??= loaded()).has(file) : !notLoaded.test(file)) return "all";
+    // it only balances shards
+    if (file === timesFile) continue;
+    if (/^(src|scripts)\/.*\.ts$/.test(file)) {
+      if (!(graph ??= loaded()).has(file)) continue;
+      const mapped = dirsFor(file);
+      if (!mapped) return "all";
+      dirs = dirs ? [...dirs, ...mapped] : mapped;
+    } else if (!notLoaded.test(file)) return "all";
+  }
+  if (dirs) {
+    // every tenth listed test, for a regression in shared code a mapped file calls into
+    for (const f of [...dirs, ...listed.filter((_, i) => i % 10 === 0)]) filters.add(f);
   }
   return [...filters];
 }

src/deflate.ts

@@ -260,10 +260,15 @@ export class Deflate {
     if (this.wrap && !this.headerDone) this.dictId = adler32(1, d) >>> 0;
     if (this.level === 0 || this.ended) return true;
     this.alloc();
-    this.resetHistory();
+    // symbols already taken keep their block; its bytes are overwritten, so it cannot be stored
+    const pending = this.nsym > 0;
+    this.head.fill(0);
+    this.matchLen = this.prevLen = MIN_MATCH - 1;
+    this.matchAvail = false;
     const t = d.length > WSIZE ? d.subarray(d.length - WSIZE) : d;
     this.win.set(t, 0);
-    this.strstart = this.blockStart = t.length;
+    this.strstart = t.length;
+    this.blockStart = pending ? -1 : t.length;
     let p = 0;
     for (; p + 2 < t.length; p++) this.insertAt(p);
     this.hashed = p;

src/fs.ts

@@ -83,8 +83,8 @@ export interface Backend {
   realpath?(path: string): string | null;
   // reports changes others make to a directory's entries (name "" is the directory itself); returns what stops it
   watch?(path: string, onEvent: (name: string, event: "create" | "modify" | "delete") => void): (() => void) | null;
-  // changes whenever the entry's content or times do, or is "" when it is missing
-  signature?(path: string): string;
+  // changes whenever the entry's content or times do, "" when it is missing, undefined when it cannot be read
+  signature?(path: string): string | undefined;
 }
 
 export interface WarmTrace {
@@ -240,16 +240,21 @@ export class VFS {
     this.warmChanged?.add(path);
     if (this.hostWatches.size === 0) return;
     const watch = this.hostWatches.get(parentOf(path));
-    if (watch) watch.own.set(path.slice(path.lastIndexOf("/") + 1), this.signature(path));
+    if (!watch) return;
+    const name = path.slice(path.lastIndexOf("/") + 1);
+    const signature = this.signature(path);
+    if (signature === undefined) watch.own.delete(name);
+    else watch.own.set(name, signature);
   }
 
   // Host directories being watched: the entries as the VM last left them, so
   // that only changes others make reach watchers a second time.
   private hostWatches = new Map<string, { stop: () => void; refs: number; own: Map<string, string> }>();
 
-  private signature(path: string): string {
+  private signature(path: string): string | undefined {
     const r = this.resolve(path);
-    return (r && r[0].signature?.(r[1])) ?? "";
+    if (!r) return "";
+    return r[0].signature ? r[0].signature(r[1]) : "";
   }
 
   // Starts reporting what others do to a directory on a host backend, as
@@ -281,7 +286,8 @@ export class VFS {
     const watch = this.hostWatches.get(dir);
     if (!watch) return;
     const path = name === "" ? dir : (dir === "/" ? "" : dir) + "/" + name;
-    if (name !== "" && watch.own.get(name) === this.signature(path)) return;
+    const signature = name === "" ? undefined : this.signature(path);
+    if (signature !== undefined && watch.own.get(name) === signature) return;
     for (const watcher of this.watchers) watcher(path, event);
   }
 

src/jvm.ts

@@ -886,7 +886,11 @@ export class JVM {
   private reportOutOfMemory(msg: string | null) {
     if (this.oomReported || !this.opts.onOutOfMemory) return;
     this.oomReported = true;
-    this.opts.onOutOfMemory(msg);
+    try {
+      this.opts.onOutOfMemory(msg);
+    } catch {
+      // the error is still thrown
+    }
   }
 
   // native: the native method throwing it, shown in its stack trace

src/natives/io.ts

@@ -327,7 +327,7 @@ export function openFile(t: JThread, path: string, write: boolean, truncate: boo
     tab.files.set(tab.next, new DirectoryFile(abs, st.size));
     return tab.next++;
   }
-  const shared = st ? [...table(t).files.values()].find((o) => o.path === abs && o.constructor === OpenFile && (o.contents.ino === undefined || o.contents.ino === st.ino))?.contents : undefined;
+  const shared = st ? [...table(t).files.values()].find((o) => o.path === abs && o.constructor === OpenFile && o.contents.ino === st.ino)?.contents : undefined;
   if (st?.device || (st && !shared && !write && st.size >= LAZY_MIN && fs.canReadRange(abs))) {
     const tab = table(t);
     tab.files.set(tab.next, st.device ? new DeviceFile(abs) : new LazyFile(abs, st.size, fs));
@@ -339,7 +339,7 @@ export function openFile(t: JThread, path: string, write: boolean, truncate: boo
     const host = openHost(fs, abs, write, readable);
     if (host) {
       closeHostsOnExit(t.vm);
-      const f = new OpenFile(abs, new Uint8Array(0), write, { data: new Uint8Array(0), size: host.size(), dirty: false, refs: 0, host, hostWritable: write, hostReadable: readable });
+      const f = new OpenFile(abs, new Uint8Array(0), write, { data: new Uint8Array(0), size: host.size(), dirty: false, refs: 0, host, hostWritable: write, hostReadable: readable, ino: st.ino });
       if (append) { f.pos = f.size; f.append = true; }
       const tab = table(t);
       tab.files.set(tab.next, f);
@@ -390,7 +390,8 @@ export function openFile(t: JThread, path: string, write: boolean, truncate: boo
 }
 
 // Only a writable host tree can lose the file while it is open, and only one
-// that reports inode numbers can reuse them.
+// that reports inode numbers can reuse them. Best effort: if no handle can be
+// taken (EMFILE), the open still succeeds, as it does on HotSpot.
 function pinInode(t: JThread, c: Contents, abs: string, write: boolean, readable: boolean) {
   const fs = t.vm.fs;
   if (c.ino === undefined || !fs.canOpenFile(abs, true)) return;

src/natives/management.ts

@@ -136,6 +136,17 @@ function procValue(file: string, label: string, field = 1): number {
 }
 
 const orElse = (x: number, fallback: number) => (Number.isFinite(x) ? x : fallback);
+
+// the soft limit, as HotSpot reads getrlimit's rlim_cur; an unlimited one is -1 after its jlong cast
+function maxOpenFiles(): bigint {
+  try {
+    const line = host("node:fs").readFileSync("/proc/self/limits", "utf8").split("\n").find((l: string) => l.startsWith("Max open files"));
+    const soft = line.trim().split(/\s+/)[3];
+    return soft === "unlimited" ? -1n : BigInt(soft);
+  } catch {
+    return 1024n;
+  }
+}
 const KB = 1024;
 
 // The VM options java-web can state, as HotSpot reports them with G1
@@ -636,7 +647,7 @@ export const natives: Natives = {
 
   [`${OS}initialize0()V`]: () => undefined,
   [`${OS}getOpenFileDescriptorCount0()J`]: (t) => BigInt(openFdCount(t.vm)),
-  [`${OS}getMaxFileDescriptorCount0()J`]: () => BigInt(orElse(procValue("/proc/self/limits", "Max open files", 3), 1024)),
+  [`${OS}getMaxFileDescriptorCount0()J`]: () => maxOpenFiles(),
   [`${OS}getCommittedVirtualMemorySize0()J`]: () => BigInt(orElse(procValue("/proc/self/statm", "", 0) * 4096, (globalThis as any).process?.memoryUsage?.().rss ?? -1)),
   [`${OS}getTotalMemorySize0()J`]: () => BigInt(host("node:os")?.totalmem() ?? MAX_HEAP),
   [`${OS}getFreeMemorySize0()J`]: () => BigInt(host("node:os")?.freemem() ?? 0),

src/node.ts

@@ -270,9 +270,13 @@ export class NodeBackend implements Backend {
       return null;
     }
   }
-  signature(p: string) {
-    const s = fs.lstatSync(this.host(p), { bigint: true, throwIfNoEntry: false });
-    return s ? `${s.ino}:${s.size}:${s.mtimeNs}` : "";
+  signature(p: string): string | undefined {
+    try {
+      const s = fs.lstatSync(this.host(p), { bigint: true, throwIfNoEntry: false });
+      return s ? `${s.ino}:${s.size}:${s.mtimeNs}` : "";
+    } catch {
+      return undefined;
+    }
   }
   setMtime(p: string, ms: number, noFollow = false, atimeMs?: number) {
     try {
@@ -407,7 +411,13 @@ function onOutError(this: NodeJS.WriteStream) {
 // What HotSpot does on the first OutOfMemoryError: it prints the commands and
 // runs each through /bin/sh, with %p the process id, before throwing.
 function runOnOutOfMemory(commands: string[], message: string | null, out: ((b: Uint8Array) => void) | undefined, fd: number, java: string[]) {
-  const say = (line: string) => out ? out(new TextEncoder().encode(line + "\n")) : fs.writeSync(fd, line + "\n");
+  // as HotSpot's fprintf: a failed write does not stop the commands
+  const say = (line: string) => {
+    try {
+      if (out) out(new TextEncoder().encode(line + "\n"));
+      else fs.writeSync(fd, line + "\n");
+    } catch { /* output closed */ }
+  };
   say(`#\n# java.lang.OutOfMemoryError: ${message ?? "null"}`);
   for (const raw of commands) {
     say(`# -XX:OnOutOfMemoryError="${raw}"`);
@@ -529,7 +539,7 @@ function snapshotCache(dir: string, build: string) {
   };
 }
 
-function nodeVm(o: LauncherOptions): JVM {
+export function nodeVm(o: LauncherOptions): JVM {
   exposeGc();
   const vfs = new VFS();
   vfs.mount("/", new NodeBackend("/", true));