diff --git a/internal/web/uploads_disconnect_handler_test.go b/internal/web/uploads_disconnect_handler_test.go index f34deab..b26ea5b 100644 --- a/internal/web/uploads_disconnect_handler_test.go +++ b/internal/web/uploads_disconnect_handler_test.go @@ -1,6 +1,7 @@ package web_test import ( + "context" "fmt" "io" "log/slog" @@ -20,48 +21,85 @@ import ( // listener (third adversarial review: over a socket every such case logged an ERROR and // half-close returned a 500, because the error page was rendered on a cancelled context). -func partialUpload(t *testing.T, addr string, hangUp func(*net.TCPConn)) { +// body positions at which the client can hang up: inside the title field, inside the file +// part's own headers, and mid-file. Every one must be handled alike (fourth adversarial +// review: the title branch had no guard and still logged an ERROR). +const ( + inTitle = "in the title" + inHeaders = "in the file part's headers" + midFile = "mid-file" +) + +func partialUpload(t *testing.T, addr, where string, hangUp func(*net.TCPConn)) { t.Helper() c, err := net.Dial("tcp", addr) require.NoError(t, err) tc := c.(*net.TCPConn) const boundary = "xBOUNDARYx" - head := "--" + boundary + "\r\nContent-Disposition: form-data; name=\"file\"; filename=\"a.m4a\"\r\nContent-Type: audio/mp4\r\n\r\n" - _, err = fmt.Fprintf(tc, "POST /uploads HTTP/1.1\r\nHost: x\r\nContent-Type: multipart/form-data; boundary=%s\r\nContent-Length: 200000\r\n\r\n%s%s", - boundary, head, strings.Repeat("a", 4000)) + var body string + switch where { + case inTitle: + body = "--" + boundary + "\r\nContent-Disposition: form-data; name=\"title\"\r\n\r\nhalf a tit" + case inHeaders: + body = "--" + boundary + "\r\nContent-Disposition: form-data; name=\"file\"; filena" + default: + body = "--" + boundary + "\r\nContent-Disposition: form-data; name=\"file\"; filename=\"a.m4a\"\r\nContent-Type: audio/mp4\r\n\r\n" + strings.Repeat("a", 4000) + } + _, err = fmt.Fprintf(tc, "POST /uploads HTTP/1.1\r\nHost: x\r\nContent-Type: multipart/form-data; boundary=%s\r\nContent-Length: 200000\r\n\r\n%s", boundary, body) require.NoError(t, err) - time.Sleep(50 * time.Millisecond) // let the server start reading the part + time.Sleep(50 * time.Millisecond) // let the server start reading hangUp(tc) } -func TestUploadPost_AClientThatHangsUpMidFileIsLoggedAtInfoAndNeverAsAnError(t *testing.T) { +func TestUploadPost_AClientThatHangsUpAnywhereInTheBodyIsLoggedAtInfoAndNeverAsAnError(t *testing.T) { hangUps := map[string]func(*net.TCPConn){ "a half close": func(c *net.TCPConn) { _ = c.CloseWrite(); time.Sleep(100 * time.Millisecond); _ = c.Close() }, "a full close": func(c *net.TCPConn) { _ = c.Close() }, "a reset (RST)": func(c *net.TCPConn) { _ = c.SetLinger(0); _ = c.Close() }, } - for name, hangUp := range hangUps { - t.Run(name, func(t *testing.T) { - app, sub := newUploadApp(t, audioOK, 200<<20) - buf := &lockedBuf{} - app.Log = slog.New(slog.NewTextHandler(buf, nil)) - srv := httptest.NewServer(app.Router()) - defer srv.Close() + for _, where := range []string{inTitle, inHeaders, midFile} { + for name, hangUp := range hangUps { + t.Run(where+", "+name, func(t *testing.T) { + app, sub := newUploadApp(t, audioOK, 200<<20) + buf := &lockedBuf{} + app.Log = slog.New(slog.NewTextHandler(buf, nil)) + srv := httptest.NewServer(app.Router()) + defer srv.Close() - partialUpload(t, strings.TrimPrefix(srv.URL, "http://"), hangUp) + partialUpload(t, strings.TrimPrefix(srv.URL, "http://"), where, hangUp) - require.Eventually(t, func() bool { + require.Eventually(t, func() bool { + out := buf.String() + return strings.Contains(out, "upload abandoned") || strings.Contains(out, "upload rejected") || strings.Contains(out, "level=ERROR") + }, 3*time.Second, 20*time.Millisecond, "the server never finished handling the aborted upload") + time.Sleep(100 * time.Millisecond) out := buf.String() - return strings.Contains(out, "upload abandoned") || strings.Contains(out, "upload rejected") || strings.Contains(out, "level=ERROR") - }, 3*time.Second, 20*time.Millisecond, "the server never finished handling the aborted upload") - time.Sleep(100 * time.Millisecond) - out := buf.String() - assert.NotContains(t, out, "level=ERROR", "a client hanging up is not a server fault:\n%s", out) - assert.Empty(t, sub.jobs) - }) + assert.NotContains(t, out, "level=ERROR", "a client hanging up is not a server fault:\n%s", out) + assert.Empty(t, sub.jobs) + }) + } } } +func TestUploadPost_AnAbandonedRequestGetsA499NotAnImplicitEmpty200(t *testing.T) { + // If the context is cancelled but the connection can still be written to (a client that + // half-closed after sending), a missing status becomes 200 with an empty body: a false + // success (fourth adversarial review). + app, _ := newUploadApp(t, audioOK, 200<<20) + buf := &lockedBuf{} + app.Log = slog.New(slog.NewTextHandler(buf, nil)) + app.Uploads = failingUploader{err: fmt.Errorf("receive: %w", io.ErrClosedPipe)} + ctx, cancel := context.WithCancel(context.Background()) + cancel() + req := uploadReq(t, "t", "a.m4a", []byte("x")).WithContext(ctx) + + rec := do(t, app, req) + + assert.Equal(t, 499, rec.Code, "never an implicit 200 for an upload that was not accepted") + assert.Contains(t, buf.String(), "upload abandoned") + assert.NotContains(t, buf.String(), "level=ERROR") +} + func TestUploadPost_ADatabaseDropThatLooksLikeAnEOFIsStillAServerError(t *testing.T) { // pgx turns a connection that drops mid-message into io.ErrUnexpectedEOF. Mapping that // sentinel to "your upload was cut off" (a 400) would hide a real fault: only an error