Check how servers handle requests that go wrong

CHANGELOG.md

@@ -34,3 +34,6 @@ Initial release.
   `load-file` in `proof serve`.
 - [#13](https://github.com/nrepl/proof/pull/13): Checks for completions and
   lookups, and nREPL's `completions` and `lookup` in `proof serve`.
+- [#14](https://github.com/nrepl/proof/pull/14): Checks for requests that go
+  wrong: frames that aren't requests, fields of the wrong type and clients
+  that go away in the middle of an eval.

PLAN.md

@@ -117,17 +117,19 @@ lookups and completions, plus requests that go wrong.
 
 ### Requests That Go Wrong
 
-- Malformed messages, top-level values that aren't dicts and fields of
-  the wrong type ([nrepl#477](https://github.com/nrepl/nrepl/issues/477)).
-- An eval whose reply bencode can't carry, e.g. a boolean
-  ([babashka#2201](https://github.com/babashka/babashka/issues/2201)).
-- A client that goes away in the middle of an eval
-  ([babashka#2227](https://github.com/babashka/babashka/issues/2227)).
-- Two clients evaluating code at the same time
-  ([jank#1022](https://github.com/jank-lang/jank/issues/1022)).
-
-After each of them the session has to keep answering, and the server as
-well.
+- Done: frames that aren't requests (the server has to keep taking
+  connections), fields of the wrong type
+  ([nrepl#477](https://github.com/nrepl/nrepl/issues/477)), a client that
+  goes away in the middle of an eval
+  ([babashka#2227](https://github.com/babashka/babashka/issues/2227)) and
+  an eval in one session holding up the others
+  ([jank#1022](https://github.com/jank-lang/jank/issues/1022)). A bencode
+  string takes jank down, and after a client goes away in the middle of
+  an eval, nREPL (1.8.0 and its main branch) never sends the `done` of
+  the next eval in that session.
+- Not done: an eval whose reply bencode can't carry, e.g. a boolean
+  ([babashka#2201](https://github.com/babashka/babashka/issues/2201)), as
+  there's no telling which code makes a server send one.
 
 ### Before the Release
 

README.md

@@ -131,14 +131,14 @@ the replies). `-like jank` turns on all of jank's scenarios at once, and
 ## Status
 
 proof is still in its early days. Right now it covers the core of the
-protocol (`describe`, unknown ops, `eval`, sessions, `stdin`, `interrupt`, `load-file`, completions, lookups and the wire format),
-what CIDER, Calva, Conjure, vim-fireplace, REPLy and neorepl send while
-connecting, evaluating code and loading files, the requests
-clients send and the server differences clients have to deal with, and
-`proof list` will show you all the checks.
-
-Checks for malformed requests are coming next, along with the first
-release.
+protocol (`describe`, unknown ops, `eval`, sessions, `stdin`,
+`interrupt`, `load-file`, completions, lookups and the wire format),
+requests that go wrong, what CIDER, Calva, Conjure, vim-fireplace, REPLy
+and neorepl send while connecting, evaluating code and loading files,
+the requests clients send and the server differences clients have to
+deal with, and `proof list` will show you all the checks.
+
+The first release is coming next.
 [PLAN.md](PLAN.md) has all the details.
 
 ## Why "proof"?

doc/design.md

@@ -194,8 +194,9 @@ checks against every scenario, and a scenario has to get the same
 verdicts as the servers it names. `-like` turns on all the scenarios of a
 server, and together they have to get that server's column of the matrix
 (apart from `eval.no-code`, which no client sends, the checks that come
-down to the server's language and those of completions and lookups,
-which are nREPL's in proof serve).
+down to the server's language, those of completions and lookups, which
+are nREPL's in proof serve, and the `robust.*` checks, as proof serve
+doesn't hang or crash where servers do).
 
 The grading works the other way around here. proof can't see what a
 client shows its users, so the client's own tests have to do that. Most

doc/profiles.md

@@ -145,7 +145,7 @@ snippet is missing, all the checks that need it are skipped.
 | `session-state` | `code`, `use`, `value` | leave some state in the session that `use` reads back as `value` | `session.across-connections`, `session.isolated` |
 | `multiple` | `code`, `values` | consist of several forms that evaluate to `values` (in order) | `eval.multiple-forms` |
 | `read-line` | `code`, `value`, `eof` | read a line from stdin, evaluating to `value` when the line is `proof` and to `eof` at the end of input | `stdin.need-input`, `stdin.roundtrip`, `stdin.eof` |
-| `sleep` | `code`, `out` | print `out` and then keep running for longer than the `timeout` (e.g. a minute), so there's something to interrupt | all the `interrupt.*` checks |
+| `sleep` | `code`, `out` | print `out` and then keep running for longer than the `timeout` (e.g. a minute), so there's something to interrupt | all the `interrupt.*` checks, `robust.disconnect`, `robust.sessions-at-once` |
 | `completion` | `code`, `value` | `code` is the start of a name, which completes to `value` | all the `completion.*` checks |
 | `lookup` | `code` | `code` is the name of a function | `lookup.lookup`, `lookup.info`, `lookup.eldoc` |
 

doc/spec-changes.md

@@ -170,7 +170,12 @@ See the decisions above.
 - Handling of malformed input - broken messages, top-level values that
   aren't dictionaries and fields of the wrong type. In
   [nrepl#477](https://github.com/nrepl/nrepl/issues/477) such a request
-  kills the session thread and later evals never get a `done`.
+  kills the session thread and later evals never get a `done`, and a
+  bencode string on its own takes jank down.
+- What happens to a session whose client goes away in the middle of an
+  eval. It should keep working for the next client
+  ([babashka#2227](https://github.com/babashka/babashka/issues/2227)),
+  and evals in other sessions shouldn't have to wait for it.
 - How the JSON in the examples maps to bencode. There are no nil or
   boolean values in bencode, so servers shouldn't try to send them
   ([nrepl#196](https://github.com/nrepl/nrepl/issues/196),

doc/usage.md

@@ -390,8 +390,10 @@ servers. There are a couple of exceptions. No scenario covers
 `eval.no-code`, as clients always send some code, and the code is still
 Clojure, even with `-like dialtone`. So the checks the profiles of
 dialtone and repartee skip for their languages (e.g. `eval.ns`) pass.
-And the completions and lookups of `proof serve` are always nREPL's
-(`completions` and `lookup`), whatever server it acts like.
+The completions and lookups of `proof serve` are always nREPL's
+(`completions` and `lookup`), whatever server it acts like, and it
+doesn't hang or crash where other servers do (see the `robust.*`
+checks).
 
 Most scenarios change only how the replies look on the wire, not what a
 user should end up seeing. Evaluating `(println "hi") (+ 1 2)` should

internal/check/check.go

@@ -12,6 +12,8 @@ package check
 import (
 	"errors"
 	"fmt"
+	"io"
+	"net"
 	"runtime"
 	"slices"
 	"sort"
@@ -125,15 +127,59 @@ func (t *T) Snippet(name string) profile.Snippet {
 
 // Connect opens a new connection, stopping the check if it can't.
 func (t *T) Connect() *nrepl.Conn {
-	c, err := nrepl.Dial(t.env.Addr, 5*time.Second)
+	c, err := t.TryConnect()
 	if err != nil {
 		t.harnessErr = fmt.Sprintf("couldn't connect to %s: %v", t.env.Addr, err)
 		t.exit()
 	}
-	t.conns = append(t.conns, c)
 	return c
 }
 
+// SendRaw sends bytes that aren't a request (or not a whole one) on a
+// connection of their own, and closes it once the server had its say. The
+// wire rules don't see that connection, as a server can't get its replies
+// to them right (e.g. there's no id to echo).
+func (t *T) SendRaw(b []byte) error {
+	nc, err := net.DialTimeout("tcp", t.env.Addr, 5*time.Second)
+	if err != nil {
+		return err
+	}
+	defer nc.Close()
+	if _, err := nc.Write(b); err != nil {
+		return err
+	}
+	// Until the server closes the connection, or goes quiet waiting for
+	// more.
+	nc.SetReadDeadline(time.Now().Add(t.settleTime()))
+	io.Copy(io.Discard, nc)
+	return nil
+}
+
+// Settle gives the server a moment to notice something it doesn't reply
+// to, e.g. a client going away. It's as long as connections settle for
+// after a check.
+func (t *T) Settle() {
+	time.Sleep(t.settleTime())
+}
+
+func (t *T) settleTime() time.Duration {
+	if t.env.Settle == 0 {
+		return defaultSettle
+	}
+	return t.env.Settle
+}
+
+// TryConnect opens a new connection, for checks that want to know whether
+// the server still takes them.
+func (t *T) TryConnect() (*nrepl.Conn, error) {
+	c, err := nrepl.Dial(t.env.Addr, 5*time.Second)
+	if err != nil {
+		return nil, err
+	}
+	t.conns = append(t.conns, c)
+	return c, nil
+}
+
 // Request sends m and waits for "done". A request that never finishes
 // fails the check outright, since every client waits for "done".
 func (t *T) Request(c *nrepl.Conn, m nrepl.Message) nrepl.Response {
@@ -244,10 +290,13 @@ type Result struct {
 
 // Run runs checks one at a time, then grades the wire traffic they
 // produced against the wire rules.
+// defaultSettle is how long connections settle when Env doesn't say.
+const defaultSettle = 100 * time.Millisecond
+
 func Run(env *Env, checks []*Check, rules []*Rule) []Result {
 	settle := env.Settle
 	if settle == 0 {
-		settle = 100 * time.Millisecond
+		settle = defaultSettle
 	}
 	results := make([]Result, len(checks))
 	conns := make([][]*nrepl.Conn, len(checks))

internal/checks/all.go

@@ -15,5 +15,7 @@ func All() []*check.Check {
 	all = append(all, completionChecks()...)
 	all = append(all, lookupChecks()...)
 	all = append(all, clientProfileChecks()...)
+	// Last, as they may take the server down.
+	all = append(all, robustChecks()...)
 	return all
 }

internal/checks/checks_test.go

@@ -77,7 +77,12 @@ func TestChecksCatchMisbehaviour(t *testing.T) {
 	connects := map[string]check.Verdict{"cider.connect": F, "calva.connect": F, "conjure.connect": F, "fireplace.connect": F,
 		"neorepl.connect": F}
 	// The checks that can't tell whether the server has their op.
-	noInterrupts, noLoadFile := skipped(interruptChecks()), skipped(loadFileChecks())
+	noLoadFile := skipped(loadFileChecks())
+	// The interrupt checks, and the ones that interrupt the sleep snippet
+	// they start.
+	noInterrupts := skipped(slices.DeleteFunc(All(), func(c *check.Check) bool {
+		return !strings.HasPrefix(c.ID, "interrupt.") && !slices.Contains(c.Requires, "interrupt.running")
+	}))
 	noLookups := skipped(completionChecks(), lookupChecks())
 	// What CIDER, Calva and Conjure send to load a file.
 	clientsLoad := map[string]check.Verdict{"cider.load-file": F, "calva.load-file": F, "conjure.load-file": F}
@@ -103,12 +108,15 @@ func TestChecksCatchMisbehaviour(t *testing.T) {
 		// Only clients put dicts and lists in their requests.
 		// Conjure's complete has extra-metadata, a list.
 		{"flat fields only", quirks{flatFields: true}, checktest.Merged(clientsLoad, map[string]check.Verdict{"completion.complete-symbol": F,
-			"cider.connect": F, "cider.eval": F, "cider.repl": F, "calva.connect": F, "calva.eval": F, "conjure.eval": F,
+			"robust.wrong-types": F,
+			"cider.connect":      F, "cider.eval": F, "cider.repl": F, "calva.connect": F, "calva.eval": F, "conjure.eval": F,
 			"neorepl.eval": F})},
 		{"any session accepted", quirks{acceptAnySession: true}, map[string]check.Verdict{"session.unknown": F, "session.closed": F}},
-		{"shared session state", quirks{sharedState: true}, map[string]check.Verdict{"session.isolated": F}},
+		// One session, so one eval at a time.
+		{"shared session state", quirks{sharedState: true}, map[string]check.Verdict{"session.isolated": F,
+			"robust.sessions-at-once": F}},
 		{"sessions tied to sockets", quirks{socketSessions: true}, map[string]check.Verdict{"session.across-connections": W,
-			"interrupt.other-connection": S}},
+			"interrupt.other-connection": S, "robust.disconnect": S}},
 		{"no ephemeral sessions", quirks{noEphemeral: true}, map[string]check.Verdict{"session.ephemeral": F, "fireplace.connect": F}},
 		{"last value only", quirks{lastValueOnly: true}, map[string]check.Verdict{"eval.multiple-forms": F, "fireplace.connect": F}},
 		{"stderr dropped", quirks{dropErr: true}, map[string]check.Verdict{"eval.stderr": F}},
@@ -135,7 +143,8 @@ func TestChecksCatchMisbehaviour(t *testing.T) {
 		{"integer value", quirks{intValue: true}, checktest.Merged(clientsLoad, map[string]check.Verdict{
 			"wire.field-types": F, "eval.value": F, "eval.survives-error": F, "eval.multiple-forms": F,
 			"session.ephemeral": F, "session.persistent": F, "cider.eval": F, "cider.repl": F, "calva.eval": F, "conjure.eval": F, "fireplace.eval": F,
-			"neorepl.eval": F, "interrupt.survives": F, "interrupt.queued": F, "load-file.value": F})},
+			"neorepl.eval": F, "interrupt.survives": F, "interrupt.queued": F, "load-file.value": F, "robust.wrong-types": W,
+			"robust.disconnect": F, "robust.sessions-at-once": F, "robust.bad-frames": F})},
 		{"unsorted keys", quirks{unsortedKeys: true}, map[string]check.Verdict{"wire.canonical": W}},
 		{"invalid UTF-8", quirks{badUTF8: true}, map[string]check.Verdict{"wire.utf8": W, "eval.stdout": F}},
 		{"no stdin op", quirks{noStdinOp: true}, map[string]check.Verdict{
@@ -158,7 +167,13 @@ func TestChecksCatchMisbehaviour(t *testing.T) {
 		// The interrupt's connection never sent the eval.
 		{"interrupted on the wrong connection", quirks{interruptHere: true}, map[string]check.Verdict{
 			"interrupt.other-connection": F, "wire.id": W}},
-		{"session dies on interrupt", quirks{diesOnInterrupt: true}, map[string]check.Verdict{"interrupt.survives": F, "interrupt.queued": F}},
+		{"session dies on interrupt", quirks{diesOnInterrupt: true}, map[string]check.Verdict{"interrupt.survives": F, "interrupt.queued": F,
+			"robust.disconnect": F}},
+		{"garbage takes the server down", quirks{crashOnGarbage: true}, map[string]check.Verdict{"robust.bad-frames": F}},
+		// The eval never gets its done, which fails even a warning.
+		{"wrong types kill the session", quirks{typesKill: true}, map[string]check.Verdict{"robust.wrong-types": F}},
+		{"a session dies with its client", quirks{diesOnDisconnect: true}, map[string]check.Verdict{"robust.disconnect": F}},
+		{"one eval at a time", quirks{oneEvalAtATime: true}, map[string]check.Verdict{"robust.sessions-at-once": F}},
 		// CIDER and Conjure send load-file without asking, and so does
 		// Calva's profile, as it can't load files either way.
 		{"no load-file op", quirks{noLoadFileOp: true}, checktest.Merged(noLoadFile, clientsLoad)},

internal/checks/fake_test.go

@@ -2,6 +2,7 @@ package checks
 
 import (
 	"bufio"
+	"errors"
 	"net"
 	"strconv"
 	"strings"
@@ -76,6 +77,10 @@ type quirks struct {
 	eldocStrings     bool // eldoc's arglists are strings rather than lists
 	pathFile         bool // the file of a symbol is a path rather than a URI
 	ciderPrefixed    bool // cider-nrepl's ops go by their cider/ names only
+	crashOnGarbage   bool // a frame that isn't a request takes the server down
+	typesKill        bool // an eval with a line that isn't an integer kills its session
+	diesOnDisconnect bool // a session dies when its eval's connection goes away
+	oneEvalAtATime   bool // an eval in one session holds up all the others
 }
 
 var unknownSession = map[string]any{"status": []any{"error", "unknown-session", "done"}}
@@ -138,15 +143,17 @@ func newFakeSession() *fakeSession {
 
 func (s *fakeServer) serve(c net.Conn) {
 	defer c.Close()
+	defer s.disconnected(c)
 	dec := bencode.NewDecoder(bufio.NewReader(c))
 	local := map[string]bool{}
 	for {
 		v, err := dec.Decode()
-		if err != nil {
-			return
-		}
 		m, ok := v.Data.(map[string]any)
-		if !ok {
+		if err != nil || !ok {
+			var garbage *bencode.SyntaxError
+			if s.q.crashOnGarbage && (err == nil || errors.As(err, &garbage)) {
+				s.ln.Close()
+			}
 			return
 		}
 		if !s.handle(c, nrepl.Message(m), local) {
@@ -155,6 +162,21 @@ func (s *fakeServer) serve(c net.Conn) {
 	}
 }
 
+// disconnected kills the sessions whose running eval came from c, with
+// diesOnDisconnect.
+func (s *fakeServer) disconnected(c net.Conn) {
+	if !s.q.diesOnDisconnect {
+		return
+	}
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	for _, sess := range s.sessions {
+		if sess.running != nil && sess.running.c == c {
+			sess.dead = true
+		}
+	}
+}
+
 func (s *fakeServer) send(c net.Conn, req nrepl.Message, fields map[string]any) {
 	out := map[string]any{}
 	_, isOut := fields["out"]
@@ -320,6 +342,16 @@ func (s *fakeServer) handle(c net.Conn, req nrepl.Message, local map[string]bool
 	return true
 }
 
+// anyRunning reports whether an eval runs in any session. s.mu is held.
+func (s *fakeServer) anyRunning() bool {
+	for _, sess := range s.sessions {
+		if sess.running != nil {
+			return true
+		}
+	}
+	return false
+}
+
 // complete offers proof for a prefix of it, like nREPL's completions and
 // cider-nrepl's complete do.
 func (s *fakeServer) complete(c net.Conn, req nrepl.Message) {
@@ -460,7 +492,7 @@ func (s *fakeServer) interrupt(c net.Conn, req nrepl.Message, local map[string]b
 	sess.running = nil
 	queue := sess.queue
 	sess.queue = nil
-	sess.dead = q.diesOnInterrupt
+	sess.dead = sess.dead || q.diesOnInterrupt
 	s.mu.Unlock()
 	to, status := run.c, []any{"done", "interrupted"}
 	if q.interruptHere {
@@ -490,7 +522,10 @@ func (s *fakeServer) eval(c net.Conn, req nrepl.Message, local map[string]bool)
 	if busy {
 		sess.queue = append(sess.queue, fakeRun{c, req, local})
 	}
-	skip := busy || sess.dead
+	if _, ok := req["line"].(string); ok && q.typesKill {
+		sess.dead = true
+	}
+	skip := busy || sess.dead || q.oneEvalAtATime && s.anyRunning()
 	s.mu.Unlock()
 	if skip {
 		return