test(web): the 499 test cancels the client mid-Accept, as a real hang-up does

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

internal/errkind/errkind.go

@@ -17,13 +17,20 @@ import (
 // HTTPStatuser is implemented by errors that are a non-2xx answer from a service.
 type HTTPStatuser interface{ HTTPStatus() int }
 
-// Of is the kind of err: "timeout", "canceled", "http_<code>", "network", or "error".
+// SQLStater is implemented by database errors that carry a SQLSTATE (pgx's PgError does).
+// The five-character code is a fixed vocabulary; the error's message is not: Postgres quotes
+// the offending value, which for a transcript row is transcript text.
+type SQLStater interface{ SQLState() string }
+
+// Of is the kind of err: "timeout", "canceled", "http_<code>", "sql_<state>", "network",
+// or "error".
 // It is empty for nil.
 func Of(err error) string {
 	if err == nil {
 		return ""
 	}
 	var hs HTTPStatuser
+	var ss SQLStater
 	var ne net.Error
 	switch {
 	case errors.Is(err, context.DeadlineExceeded):
@@ -32,6 +39,8 @@ func Of(err error) string {
 		return "canceled"
 	case errors.As(err, &hs):
 		return fmt.Sprintf("http_%d", hs.HTTPStatus())
+	case errors.As(err, &ss) && ss.SQLState() != "":
+		return "sql_" + ss.SQLState()
 	case errors.As(err, &ne):
 		if ne.Timeout() {
 			return "timeout"

internal/errkind/errkind_test.go

@@ -54,6 +54,24 @@ func TestOf_ReducesAnErrorToAKindAndNeverItsText(t *testing.T) {
 	}
 }
 
+type sqlErr struct{}
+
+func (sqlErr) Error() string {
+	return `ERROR: invalid input syntax for type json: "Anders ansvarar för virket"`
+}
+func (sqlErr) SQLState() string { return "22P02" }
+
+func TestOf_ADatabaseErrorIsReducedToItsSQLStateNeverItsMessage(t *testing.T) {
+	// A Postgres error can quote the offending value, which for a transcript row is
+	// transcript text. The five-character SQLSTATE is a fixed vocabulary (fourth review).
+	err := fmt.Errorf("store: complete upload: %w", sqlErr{})
+
+	got := errkind.Of(err)
+
+	assert.Equal(t, "sql_22P02", got)
+	assert.NotContains(t, got, "Anders")
+}
+
 func TestOf_AKindCarriesNoWordFromTheError(t *testing.T) {
 	err := fmt.Errorf("convert /spool/upload-555: %w", errors.New("Anders ansvarar för virket"))
 

internal/uploads/intake_busy_test.go

@@ -78,7 +78,7 @@ func (b *brokenBody) Read(p []byte) (int, error) {
 	return 0, io.ErrUnexpectedEOF
 }
 
-func TestAccept_AnErrorReadingTheClientsBodyIsMarkedAsTheClientsAndNothingElseIs(t *testing.T) {
+func TestAccept_AnErrorReadingTheClientsBodyIsMarkedAsTheClientsAndAWrappedSubmitErrorIsNot(t *testing.T) {
 	r := newRig(t)
 
 	_, err := r.in.Accept(context.Background(), "user-1", "t", &brokenBody{n: 1})

internal/uploads/worker.go

@@ -242,7 +242,7 @@ func (w *Worker) Process(ctx context.Context, job Job) {
 	start := time.Now()
 
 	if err := w.store.StartTranscribing(ctx, job.UserID, job.UploadID); err != nil {
-		log.Error("upload: start transcribing", "err", err)
+		log.Error("upload: start transcribing", "kind", errkind.Of(err))
 		w.fail(ctx, job, log, domain.UploadErrTranscriptionFailed)
 		return
 	}
@@ -279,7 +279,7 @@ func (w *Worker) Process(ctx context.Context, job Job) {
 
 	wav, err := w.tempWAV()
 	if err != nil {
-		log.Error("upload: temp wav", "err", err)
+		log.Error("upload: temp wav", "kind", errkind.Of(err))
 		abort(domain.UploadErrTranscriptionFailed)
 		return
 	}
@@ -322,7 +322,7 @@ func (w *Worker) Process(ctx context.Context, job Job) {
 
 	tr.Begin("save", 0)
 	if err := w.store.CompleteUpload(ctx, job.UserID, job.UploadID, t); err != nil {
-		log.Error("upload: store transcript", "err", err)
+		log.Error("upload: store transcript", "kind", errkind.Of(err))
 		abort(domain.UploadErrTranscriptionFailed)
 		return
 	}
@@ -339,7 +339,7 @@ func (w *Worker) fail(ctx context.Context, job Job, log *slog.Logger, kind domai
 		return
 	}
 	if err := w.store.FailUpload(ctx, job.UserID, job.UploadID, kind); err != nil {
-		log.Error("upload: record failure", "err", err)
+		log.Error("upload: record failure", "kind", errkind.Of(err))
 	}
 }
 

internal/uploads/worker_progress_test.go

@@ -197,3 +197,19 @@ func TestWorkerProgress_AFailureLogsAKindNeverTheErrorTextOrThePath(t *testing.T
 		})
 	}
 }
+
+func TestWorkerProgress_AStoreFailureLogsItsSQLStateNeverTheValueItQuotes(t *testing.T) {
+	// A Postgres error can quote the offending value; for a transcript row that is the
+	// transcript (fourth adversarial review). The log gets the SQLSTATE.
+	r := newWRig(t, time.Minute)
+	r.st.completeErr = dbErr{}
+	buf := &syncLog{}
+	w := newProgressWorker(t, r, r.tr, uploads.WithLogger(slog.New(slog.NewTextHandler(buf, nil))))
+
+	w.Process(context.Background(), r.spool(t, "a"))
+
+	out := buf.String()
+	assert.NotContains(t, out, "zebrakaka")
+	assert.Contains(t, out, "upload: store transcript")
+	assert.Contains(t, out, "kind=sql_22P02")
+}

internal/uploads/worker_test.go

@@ -324,3 +324,11 @@ func TestWorkerRun_StoppingDeletesQueuedAudioAndRefusesNewJobs(t *testing.T) {
 	assert.Empty(t, r.leftovers(t), "audio of queued jobs is deleted on stop: the rows are swept to interrupted at next start")
 	assert.ErrorIs(t, r.w.Submit(r.spool(t, "c")), uploads.ErrStopped)
 }
+
+// dbErr is an error shaped like pgx's: a message that quotes a value, and an SQLSTATE.
+type dbErr struct{}
+
+func (dbErr) Error() string {
+	return `ERROR: invalid input syntax for type json: "zebrakaka secret words"`
+}
+func (dbErr) SQLState() string { return "22P02" }

internal/web/uploads.go

@@ -95,7 +95,7 @@ func (a *App) handleUploadPost(w http.ResponseWriter, r *http.Request) {
 		}
 		if err != nil {
 			var tooBig *http.MaxBytesError
-			if a.abandoned(r) {
+			if a.abandoned(w, r) {
 				return
 			}
 			if !errors.As(err, &tooBig) {
@@ -113,6 +113,9 @@ func (a *App) handleUploadPost(w http.ResponseWriter, r *http.Request) {
 		case "title":
 			b, err := io.ReadAll(io.LimitReader(part, maxTitleRunes*utf8.UTFMax))
 			if err != nil {
+				if a.abandoned(w, r) {
+					return
+				}
 				a.renderUploadsPage(w, r, userID, http.StatusBadRequest, "That upload could not be read. Please try again.")
 				return
 			}
@@ -133,14 +136,17 @@ func (a *App) handleUploadPost(w http.ResponseWriter, r *http.Request) {
 	a.renderUploadsPage(w, r, userID, http.StatusBadRequest, "Choose an audio file to upload.")
 }
 
-// abandoned reports, and logs at INFO, that the client has already gone: its request
+// abandoned reports, logs at INFO and answers 499, because the client has already gone: its request
 // context is cancelled, so there is nobody to answer and rendering a page would run its
 // database read on a dead context and log a false ERROR. A hang-up mid-upload is routine.
-func (a *App) abandoned(r *http.Request) bool {
+func (a *App) abandoned(w http.ResponseWriter, r *http.Request) bool {
 	if r.Context().Err() == nil {
 		return false
 	}
 	a.logger().Info("upload abandoned", "reason", "client_gone")
+	// 499 (the "client closed request" convention): a client that half-closed after sending
+	// can still read, and an unwritten status would reach it as a false, empty 200.
+	w.WriteHeader(499)
 	return true
 }
 
@@ -148,7 +154,7 @@ func (a *App) abandoned(r *http.Request) bool {
 // rejection is the user's to fix (422, or 413 for size) and carries its own
 // message; anything else is ours.
 func (a *App) uploadFailure(w http.ResponseWriter, r *http.Request, userID string, err error) {
-	if a.abandoned(r) {
+	if a.abandoned(w, r) {
 		return
 	}
 	var rej *uploads.RejectError

internal/web/uploads_disconnect_handler_test.go

@@ -1,6 +1,7 @@
 package web_test
 
 import (
+	"context"
 	"fmt"
 	"io"
 	"log/slog"
@@ -20,35 +21,52 @@ 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 _, where := range []string{inTitle, inHeaders, midFile} {
 		for name, hangUp := range hangUps {
-		t.Run(name, func(t *testing.T) {
+			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 {
 					out := buf.String()
@@ -60,6 +78,34 @@ func TestUploadPost_AClientThatHangsUpMidFileIsLoggedAtInfoAndNeverAsAnError(t *
 				assert.Empty(t, sub.jobs)
 			})
 		}
+	}
+}
+
+// goneMidAccept is an Uploader during whose Accept the client disappears: the request's
+// context is cancelled, then Accept fails the way a read from a dead connection does.
+type goneMidAccept struct{ cancel context.CancelFunc }
+
+func (g goneMidAccept) Accept(context.Context, string, string, io.Reader) (string, error) {
+	g.cancel()
+	return "", fmt.Errorf("receive: %w", io.ErrClosedPipe)
+}
+
+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))
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+	app.Uploads = goneMidAccept{cancel: cancel}
+
+	rec := do(t, app, uploadReq(t, "t", "a.m4a", []byte("x")).WithContext(ctx))
+
+	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) {