Skip to content

Contain backend OSErrors at the command-execution boundary - #2

Open
ninowalker wants to merge 1 commit into
mainfrom
fix/oserror-command-boundary
Open

Contain backend OSErrors at the command-execution boundary#2
ninowalker wants to merge 1 commit into
mainfrom
fix/oserror-command-boundary

Conversation

@ninowalker

Copy link
Copy Markdown
Owner

Implements option (a) from dbreunig#9, with a little of option (b) alongside it.

The problem

Commands catch a narrow, specific set of filesystem errors — cat catches exactly FileNotFoundError, IsADirectoryError and PermissionError — so a custom backend that raises anything else propagates out of bash.exec() as a Python exception instead of becoming a nonzero exit with a message on stderr. A network-backed filesystem has no exception to fail through: a timeout, a connection reset or a plain OSError all escape. That breaks the containment property the sandbox exists to provide, and pushes a try/except around every exec() call into every harness. The redirect path already handles this correctly (redirection failures are wrapped and become exit 1), so the command path is the inconsistency.

The design

One catch at the command-execution boundary, not per-command. _execute_simple_command already has a try/finally spanning command-name expansion, builtin dispatch, command dispatch and output redirection — the point where a simple command's ExecResult is produced. Adding an except OSError clause to it is a single handler covering all of them, including builtins like test -L that reach the filesystem without going through a command object. The existing per-command handlers are untouched and still run first, so cat: /x: No such file or directory is still phrased by cat; the backstop only ever sees what no command claimed.

OSError only, deliberately — this is not except Exception. The line is between the environment failing and the code being wrong:

  • An OSError is the environment failing. A shell contains that and reports it as a nonzero exit. TimeoutError and ConnectionError are OSError subclasses, so remote and network-backed filesystems arrive here for free — no new taxonomy to invent.
  • Anything else is a bug in the backend, and a bug should crash loudly. Swallowing AttributeError or TypeError into exit 1 would turn a broken backend into a shell that quietly misbehaves, which is strictly worse than a traceback.

The interpreter's own control flow — ExitError, ReturnError, BreakError, ContinueError, ErrexitError — derives from InterpreterError(Exception), not OSError, so the backstop structurally cannot eat it. Tests pin that.

Message shape is <cmd>: <filename>: <strerror> when the error carries a filename, <cmd>: <strerror or str(e)> when it does not, on stderr with exit code 1. Because it is an ordinary nonzero ExecResult, it composes with everything already built on exit codes: pipelines take the failing segment's code, set -e stops, || and && branch normally.

$ cat /flaky.txt          # backend raises OSError(EIO, "Input/output error", "/flaky.txt")
cat: /flaky.txt: Input/output error
$ echo $?
1

The tests

tests/test_interpreter/test_fs_error_boundary.py uses a FlakyFs wrapper that delegates to InMemoryFs and raises a chosen error on chosen paths. Ten cases in three groups:

  • Containment — an OSError with an errno and filename, and a bare TimeoutError, each become exit 1 with the expected stderr and do not raise; the shell keeps running afterwards; set -e stops on it. These four fail without the change (the exception escapes bash.exec()).
  • No behaviour changeENOENT, EISDIR and EACCES still produce cat's own coreutils-accurate messages, not the generic one.
  • Control flowexit 3 still yields 3, break still leaves the loop, return 4 still sets $?.

The six tests in the last two groups pass with and without the change, which is the point: the backstop adds a path and moves nothing that already worked.

Docs

One paragraph under a new Custom Filesystems heading in the README, next to the other filesystem docs where a backend author is already reading: raise OSError subclasses; the three recognized ones get command-specific messages; everything else is formatted generically by the boundary; non-OSError propagates because it means something is broken.

Scope

Input redirection for simple commands (< file) is handled inline before this try block and keeps its existing FileNotFoundError/IsADirectoryError handling — extending the boundary upward to cover it would mean re-indenting about sixty lines of deeply nested code, so I left it out rather than bury the change in a reformat. Happy to do it separately if it's wanted.

Testing

  • pytest tests/test_commands/ tests/test_network.py (the CI command): 2314 passed, 1 skipped
  • everything except tests/spec_tests and tests/comparison_tests: 2923 passed, 3 skipped
  • tests/spec_tests + tests/comparison_tests: identical results to main (pre-existing failures, unchanged count)

Note

Staged here on the fork rather than sent upstream, pending the design conversation on dbreunig#9. Intentionally no Fixes line so this closes nothing.

A custom filesystem backend that raises anything outside the three errors
the commands recognize propagates out of bash.exec() as a Python
exception instead of a nonzero exit. Network-backed backends have no
exception to fail through at all.

Add one except OSError clause to the try/finally in
_execute_simple_command, which spans builtin dispatch, command dispatch
and output redirection. It reports "<cmd>: <filename>: <strerror>" and
exit 1, and only sees errors no command handled, so existing messages are
unchanged. OSError only: the interpreter's control-flow exceptions derive
from InterpreterError and cannot be swallowed, and a non-OSError means the
backend is broken and should crash rather than become exit 1.

Document the expected exception taxonomy for backend authors in the README.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant