fix(code-execution): the advice after a signal fits the signal: a crash's points at native code, and only SIGKILL keeps the out-of-memory example

changelog.d/fix-code-execution-signal-advice.fixed.md

@@ -0,0 +1 @@
+- When a crash's signal (SIGSEGV, SIGABRT, SIGBUS, SIGFPE or SIGILL) ends `code_execution`'s Python interpreter while a script runs, the result now says to check the traceback above it for the call that crashed in native code, such as an extension module the script uses, or, when no traceback reached the result, to check whether native code the script calls crashes. It used to say to check whether the script uses too much memory, which now comes only after SIGKILL, the signal the out-of-memory killer sends. Any other signal gets `os.kill()` as its example.

src/code-execution/py-runner.ts

@@ -581,6 +581,12 @@ function stoppedReading(background: boolean): string {
     + "shuts down or replaces file descriptor 3, where the host's messages arrive; if not, ask whoever runs this host.";
 }
 
+/** The signals faulthandler writes a traceback for (runtime-py.ts), which a crash ends a process with. */
+const CRASH_SIGNALS: ReadonlySet<string> = new Set(['SIGSEGV', 'SIGABRT', 'SIGBUS', 'SIGFPE', 'SIGILL']);
+
+/** How faulthandler's traceback for a crash's signal begins. */
+const CRASH_TRACEBACK = 'Fatal Python error: ';
+
 /**
  * Why a script stopped when its interpreter exited under it, and what running it
  * again does; or, when the script's result came before the exit (ended), that
@@ -593,9 +599,20 @@ function interpreterExit(exitCode: number | null, signal: NodeJS.Signals | null,
   // copy of fd 2 and fd 2 itself, or shut down the socket under them, and it's read from what the result
   // keeps, which past a stream's cap or a journal's tail lacks it.
   const taken = exitCode === RUNTIME_FATAL_EXIT ? fdTakenFromRuntime(said) : null;
+  // After a crash's signal, faulthandler's traceback shows the call that crashed, in native code or the
+  // script's own os.abort(), when it reached what the result keeps: a script can disable faulthandler,
+  // and a stream's cap or a journal's tail can lose it. Its first words are looked for anywhere, not
+  // only at a line's start: faulthandler doesn't start a line, so after output that didn't end one,
+  // such as a progress bar's, they follow on that line. SIGKILL is the out-of-memory killer's; any
+  // other signal, the script can send itself.
   const check = taken
     ? `check whether the script closes, shuts down or replaces ${taken}, as the runtime's line above says`
-    : `check whether the script itself ends the process, for example ${signal ? 'by using too much memory' : 'with os._exit()'}`;
+    : signal && CRASH_SIGNALS.has(signal)
+      ? said.includes(CRASH_TRACEBACK)
+        ? 'check the traceback above for the call that crashed in native code, such as an extension module the script uses'
+        : 'check whether native code the script calls crashes, such as an extension module it uses'
+      : `check whether the script itself ends the process, for example ${
+        signal === 'SIGKILL' ? 'by using too much memory' : signal ? 'with os.kill()' : 'with os._exit()'}`;
   const gone = background ? '' : ', and variables from earlier scripts are gone';
   if (ended) {
     return `script ended, but the Python interpreter ${how} before all of its output reached the host, so some of it may be missing${gone}. `

test/code-execution.test.ts

@@ -66,24 +66,33 @@ const alive = (pid: number): boolean => {
 };
 
 /** What a script's result says when its interpreter exits under it (py-runner's
- *  interpreterExit), for `how` such as "exited with code 3" or "was ended by SIGKILL". */
-const exitedUnder = (how: string, background = false): string =>
+ *  interpreterExit), for `how` such as "exited with code 3" or "was ended by SIGKILL",
+ *  with `check` for the advice's first step after a crash's signal. */
+const exitedUnder = (how: string, background = false, check?: string): string =>
   `script stopped: the Python interpreter ${how} while it ran, so the rest of the script didn't run`
   + `${background ? '' : ', and variables from earlier scripts are gone'}. Tool calls it had already made keep their effects, `
-  + 'so check them before you run it again in a new interpreter. If it stops the same way, check whether the script itself '
-  + `ends the process, for example ${how.startsWith('was ended by') ? 'by using too much memory' : 'with os._exit()'}; if not, ask whoever runs this host.`;
+  + `so check them before you run it again in a new interpreter. If it stops the same way, ${check ?? 'check whether the script itself '
+  + `ends the process, for example ${
+    how === 'was ended by SIGKILL' ? 'by using too much memory' : how.startsWith('was ended by') ? 'with os.kill()' : 'with os._exit()'}`
+  }; if not, ask whoever runs this host.`;
+
+/** The advice's first step after a crash's signal, with faulthandler's traceback above it, and without. */
+const NATIVE_CRASH = 'check the traceback above for the call that crashed in native code, such as an extension module the script uses';
+const NATIVE_CRASH_UNSEEN = 'check whether native code the script calls crashes, such as an extension module it uses';
 
 /**
  * What a script's result says when its interpreter exits after the script's own
  * result came, before all of its output arrived (py-runner's interpreterExit,
- * ended), with the advice for a runtime that ended itself over `taken`, if given.
+ * ended), with the advice for a runtime that ended itself over `taken`, if given,
+ * and `check` for the advice's first step after a crash's signal, as exitedUnder's.
  */
-const exitedAfterEnd = (how: string, taken?: string, background = false): string =>
+const exitedAfterEnd = (how: string, taken?: string, background = false, check?: string): string =>
   `script ended, but the Python interpreter ${how} before all of its output reached the host, so some of it may be missing`
   + `${background ? '' : ', and variables from earlier scripts are gone'}. If it happens again, `
   + (taken
     ? `check whether the script closes, shuts down or replaces ${taken}, as the runtime's line above says`
-    : `check whether the script itself ends the process, for example ${how.startsWith('was ended by') ? 'by using too much memory' : 'with os._exit()'}`)
+    : check ?? `check whether the script itself ends the process, for example ${
+      how === 'was ended by SIGKILL' ? 'by using too much memory' : how.startsWith('was ended by') ? 'with os.kill()' : 'with os._exit()'}`)
   + '; if not, ask whoever runs this host.';
 
 /** The result's text after the runtime ended itself because a script took one of its file descriptors. */
@@ -1458,6 +1467,38 @@ describe("PyRunner: a script's output, read by the host (real python3)", { skip:
     }
   });
 
+  it("a signal's advice fits it: the traceback's call after a crash's signal, native code when no traceback came, the script's own os.kill() after another", async () => {
+    // Each with where faulthandler's traceback begins in the result, above the advice, or null where none came.
+    const header = /^Fatal Python error: /m;
+    const cases: [string, string, RegExp | null, string][] = [
+      ['a segfault', 'import ctypes\nctypes.string_at(0)', header, exitedUnder('was ended by SIGSEGV', false, NATIVE_CRASH)],
+      ['a fatal error', 'import ctypes\nctypes.pythonapi.Py_FatalError(b"probe reason")', header, exitedUnder('was ended by SIGABRT', false, NATIVE_CRASH)],
+      ...['SIGBUS', 'SIGFPE', 'SIGILL'].map((sig): [string, string, RegExp, string] =>
+        [sig, `import os, signal\nos.kill(os.getpid(), signal.${sig})`, header, exitedUnder(`was ended by ${sig}`, false, NATIVE_CRASH)]),
+      // faulthandler doesn't start a line: after a progress bar's, which doesn't end, its first words follow on it.
+      ["a segfault after a line the script didn't end", 'import sys, ctypes\nsys.stderr.write("\\r 45%|####      | 45/100")\nsys.stderr.flush()\nctypes.string_at(0)',
+        /45\/100Fatal Python error: /, exitedUnder('was ended by SIGSEGV', false, NATIVE_CRASH)],
+      ['a segfault with faulthandler disabled', 'import ctypes, faulthandler\nfaulthandler.disable()\nctypes.string_at(0)', null,
+        exitedUnder('was ended by SIGSEGV', false, NATIVE_CRASH_UNSEEN)],
+      ['SIGTERM', 'import os, signal\nos.kill(os.getpid(), signal.SIGTERM)', null, exitedUnder('was ended by SIGTERM')],
+    ];
+    for (const [what, code, traceback, says] of cases) {
+      const runner = new PyRunner({ onToolCall: async () => '' });
+      try {
+        const result = await within(runner.exec(code, []), what);
+        assert.strictEqual(result.aborted, true, what);
+        if (traceback) {
+          assert.match(result.stderr, traceback, what);
+          assert.ok(result.stderr.endsWith(`\n${says}`), `${what}: ${JSON.stringify(result.stderr.slice(-600))}`);
+        } else {
+          assert.strictEqual(result.stderr, says, what);
+        }
+      } finally {
+        runner.dispose();
+      }
+    }
+  });
+
   it('an interpreter that ends under its script leaves in the result all it wrote', async () => {
     const runner = new PyRunner({ onToolCall: async () => '' });
     try {