Drive the archive reopen debounce test from a clock (closes #190)

The archive writer now reads the time its reopen debounce is
measured on from a clock field, time.Now outside tests. The
debounce test moves that clock instead of sleeping past a real
2 second window, so a slow host between the two quick writes can
no longer turn a correct result red.

Model: opus-5-5

.gitea/workflows/check.yml

@@ -33,5 +33,5 @@ jobs:
         # report success from cache.
         run: git rev-parse HEAD > .ci-fingerprint
 
-      - name: Build Docker image (runs make check)
+      - name: Build Docker image (runs make fmt-check, golangci-lint, make test, make build)
         run: script/cibuild

README.md

@@ -19,8 +19,8 @@ before deploying one.
 ### Prerequisites
 
 - Go 1.26.1+ (the version in `go.mod`)
-- Docker (for linting, for the test stage of the CI gate, and for
+- Docker (for `make lint` and so for `make check`, for the CI gate, and
-  containerized deployment)
+  for containerized deployment)
 
 golangci-lint is not a prerequisite and must not be installed on the
 host: `script/bootstrap` does not install it, and `make lint` runs the
@@ -3335,8 +3335,9 @@ linked, which is what lets it run on the Alpine runtime image.
 inside the image, so a build that succeeds is a repo that is formatted,
 linted, tested and compiled. `script/lint` also uses Docker
 (`Dockerfile.lint`, see Linting above), so `make lint` and `make check`
-run the same pinned linter version the gate does; only `script/test`
+run the same pinned linter version the gate does; of the steps
-and `script/fmt-check` run on the host.
+`make check` runs, only `script/test` and `script/fmt-check` run on the
+host.
 
 #### CI gate honesty
 

internal/delivery/target_database_test.go

@@ -191,9 +191,9 @@ func TestArchiveWriter_ReopenDebounce(t *testing.T) {
 		path, archiveTestLogger(), debounce,
 	)
 
-	// The writer reads the time from this clock, which only the
+	// The writer measures its reopen debounce on this clock, which
-	// test moves, so how long the host takes between writes
+	// only the test moves, so how long the host takes between
-	// cannot change the result.
+	// writes cannot change the result.
 	now := time.Now()
 
 	w.SetNow(func() time.Time { return now })

internal/handlers/auth.go

@@ -139,8 +139,7 @@ func (h *Handlers) renderLoginError(
 		),
 	}
 
-	w.WriteHeader(status)
+	h.renderTemplateStatus(w, r, "login.html", data, status)
-	h.renderTemplate(w, r, "login.html", data)
 }
 
 // authenticateUser looks up and verifies a user's credentials.

internal/handlers/auth_test.go

@@ -3,6 +3,7 @@ package handlers_test
 import (
 	"context"
 	"fmt"
+	"html/template"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
@@ -404,6 +405,60 @@ func TestLogin_MissingCredentialsRejectedBeforeAnyHash(t *testing.T) {
 	)
 }
 
+// TestLogin_FormErrorAnswersItsStatusWithThePage proves that the login
+// form shown again with an error still answers 400 with the whole page.
+func TestLogin_FormErrorAnswersItsStatusWithThePage(t *testing.T) {
+	t.Parallel()
+
+	var h *handlers.Handlers
+
+	app := newTestApp(t, &h)
+	app.RequireStart()
+
+	t.Cleanup(app.RequireStop)
+
+	w := submitLogin(h, sharedProxyPeer, "", "")
+
+	assert.Equal(t, http.StatusBadRequest, w.Code)
+	assert.Contains(
+		t, w.Body.String(), "Username and password are required",
+	)
+	assert.Contains(
+		t, w.Body.String(), "</html>",
+		"the page must render to completion",
+	)
+}
+
+// TestLogin_FormErrorRenderFailureAnswers500 proves that a login form
+// error page whose template fails answers 500 with the error page and
+// none of the form page, rather than the 400 it meant to send.
+func TestLogin_FormErrorRenderFailureAnswers500(t *testing.T) {
+	t.Parallel()
+
+	var h *handlers.Handlers
+
+	app := newTestApp(t, &h)
+	app.RequireStart()
+
+	t.Cleanup(app.RequireStop)
+
+	// The page prints its error message and then fails.
+	h.AddTemplateForTest("login.html", template.Must(
+		template.New("login").Funcs(template.FuncMap{
+			"fail": func() (string, error) { return "", errMidRender },
+		}).Parse(`{{.Error}}{{fail}}`),
+	))
+
+	w := submitLogin(h, sharedProxyPeer, "", "")
+
+	assert.Equal(t, http.StatusInternalServerError, w.Code)
+	assert.NotContains(
+		t, w.Body.String(), "Username and password are required",
+		"the response must carry no part of the aborted page",
+	)
+	assert.Contains(t, w.Body.String(), "500 Internal Server Error")
+}
+
 // TestLogin_SuccessCreatesSession is the control for the tests above:
 // the success path they assert on really does authenticate.
 func TestLogin_SuccessCreatesSession(t *testing.T) {

internal/handlers/handlers.go

@@ -309,12 +309,26 @@ func (s *Handlers) getUserInfo(
 }
 
 // renderTemplate renders a pre-parsed template with common
-// data
+// data and answers 200.
 func (s *Handlers) renderTemplate(
 	w http.ResponseWriter,
 	r *http.Request,
 	pageTemplate string,
 	data any,
+) {
+	s.renderTemplateStatus(w, r, pageTemplate, data, http.StatusOK)
+}
+
+// renderTemplateStatus is renderTemplate answering with status, for a
+// form shown again with an error. Call it instead of WriteHeader
+// followed by renderTemplate: the status is written only once the page
+// has rendered, so a failed render can still answer 500.
+func (s *Handlers) renderTemplateStatus(
+	w http.ResponseWriter,
+	r *http.Request,
+	pageTemplate string,
+	data any,
+	status int,
 ) {
 	tmpl, ok := s.templates[pageTemplate]
 	if !ok {
@@ -327,7 +341,9 @@ func (s *Handlers) renderTemplate(
 		return
 	}
 
-	s.executeTemplate(w, r, tmpl, s.pageData(r, data, noticeFor(r)))
+	s.executeTemplate(
+		w, r, tmpl, s.pageData(r, data, noticeFor(r)), status,
+	)
 }
 
 // pageData adds the fields the shared layout renders to a page's own
@@ -363,19 +379,20 @@ func (s *Handlers) pageData(
 	}
 }
 
-// executeTemplate renders the template into a buffer and writes to
+// executeTemplate renders the template into a buffer and writes status
-// the response only once rendering has fully succeeded. Executing
+// and the page to the response only once rendering has fully
-// straight into the ResponseWriter commits a partial body and a 200
+// succeeded. Executing straight into the ResponseWriter commits a
-// status before a mid-render error can be reported, leaving no way
+// partial body and the status before a mid-render error can be
-// to serve a 500. Buffering makes a page's rendered size resident
+// reported, leaving no way to serve a 500. Buffering makes a page's
-// memory per concurrent viewer, so every page owes it a bound: the
+// rendered size resident memory per concurrent viewer, so every page
-// event log caps each stored body at maxRenderedBodyBytes for exactly
+// owes it a bound: the event log caps each stored body at
-// this reason.
+// maxRenderedBodyBytes for exactly this reason.
 func (s *Handlers) executeTemplate(
 	w http.ResponseWriter,
 	r *http.Request,
 	tmpl *template.Template,
 	data any,
+	status int,
 ) {
 	var buf bytes.Buffer
 
@@ -390,6 +407,7 @@ func (s *Handlers) executeTemplate(
 	}
 
 	w.Header().Set("Content-Type", "text/html; charset=utf-8")
+	w.WriteHeader(status)
 
 	_, err = buf.WriteTo(w)
 	if err != nil {

internal/handlers/source_management.go

@@ -350,12 +350,12 @@ func (h *Handlers) HandleSourceCreateSubmit() http.HandlerFunc {
 		retentionStr := r.PostFormValue("retention_days")
 
 		if name == "" {
-			w.WriteHeader(http.StatusBadRequest)
+			h.renderTemplateStatus(
-			h.renderTemplate(
 				w, r, "sources_new.html",
 				newSourceFormData(
 					"Name is required", name, description,
 				),
+				http.StatusBadRequest,
 			)
 
 			return
@@ -365,13 +365,13 @@ func (h *Handlers) HandleSourceCreateSubmit() http.HandlerFunc {
 			retentionStr, database.DefaultRetentionDays,
 		)
 		if retErr != nil {
-			w.WriteHeader(http.StatusBadRequest)
+			h.renderTemplateStatus(
-			h.renderTemplate(
 				w, r, "sources_new.html",
 				newSourceFormData(
 					retentionErrorMessage(retErr),
 					name, description,
 				),
+				http.StatusBadRequest,
 			)
 
 			return
@@ -644,8 +644,7 @@ func (h *Handlers) applyWebhookEdit(
 			tmplKeyError:   "Name is required",
 		}
 
-		w.WriteHeader(http.StatusBadRequest)
+		h.renderTemplateStatus(w, r, "source_edit.html", data, http.StatusBadRequest)
-		h.renderTemplate(w, r, "source_edit.html", data)
 
 		return
 	}
@@ -665,8 +664,7 @@ func (h *Handlers) applyWebhookEdit(
 			tmplKeyError:   retentionErrorMessage(retErr),
 		}
 
-		w.WriteHeader(http.StatusBadRequest)
+		h.renderTemplateStatus(w, r, "source_edit.html", data, http.StatusBadRequest)
-		h.renderTemplate(w, r, "source_edit.html", data)
 
 		return
 	}
@@ -703,8 +701,7 @@ func (h *Handlers) applyWebhookEdit(
 					"it, then save again.",
 			}
 
-			w.WriteHeader(http.StatusConflict)
+			h.renderTemplateStatus(w, r, "source_edit.html", data, http.StatusConflict)
-			h.renderTemplate(w, r, "source_edit.html", data)
 
 			return
 		}
@@ -1911,9 +1908,13 @@ func (h *Handlers) HandleTargetToggle() http.HandlerFunc {
 				return false, err
 			}
 
-			tgt.Active = !tgt.Active
+			// Only the active column: saving the whole row would
+			// write back the name and settings read above over an
+			// edit saved since.
+			active := !tgt.Active
 
-			return tgt.Active, h.db.DB().Save(&tgt).Error
+			return active, h.db.DB().Model(&tgt).
+				Update("active", active).Error
 		},
 		"failed to toggle target",
 		targetActivated, targetDeactivated,

internal/handlers/target_toggle_test.go

@@ -0,0 +1,66 @@
+package handlers_test
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"gorm.io/gorm"
+)
+
+// TestHandleTargetToggle_DoesNotUndoAnEdit proves that a toggle which
+// loaded the target before an edit of it was saved does not write the
+// old name and settings back over the edit. The edit is submitted from
+// a callback on the toggle's own read of the target, so it is saved
+// after that read and before the toggle writes.
+func TestHandleTargetToggle_DoesNotUndoAnEdit(t *testing.T) {
+	t.Parallel()
+
+	env := setupSourceTest(t)
+	wh, tgt := seedHTTPTarget(t, env, "", "")
+	require.True(t, tgt.Active)
+
+	var (
+		edited   bool
+		editCode int
+	)
+
+	require.NoError(t, env.db.DB().Callback().Query().
+		After("gorm:query").
+		Register("test:edit_after_toggle_read", func(tx *gorm.DB) {
+			// The edit reads the target too; only the toggle's read,
+			// the first, submits it.
+			if tx.Statement.Table != "targets" || edited {
+				return
+			}
+
+			edited = true
+			editCode = submitTargetEdit(
+				env, wh.ID, tgt.ID,
+				editForm(editReplacedURL, "", ""),
+			).Code
+		}),
+	)
+
+	req := postRequest(
+		"/hook/"+wh.ID+"/targets/"+tgt.ID+"/toggle",
+		env.cookies,
+		map[string]string{paramSourceID: wh.ID, paramTargetID: tgt.ID},
+	)
+	w := httptest.NewRecorder()
+
+	env.handlers.HandleTargetToggle().ServeHTTP(w, req)
+
+	require.Equal(t, http.StatusSeeOther, w.Code)
+	require.Equal(t, http.StatusSeeOther, editCode)
+
+	stored := storedTarget(t, env, tgt.ID)
+	assert.False(t, stored.Active)
+	assert.Equal(t, "edited-name", stored.Name)
+	assert.Equal(t, 5, stored.MaxRetries)
+	assert.Equal(
+		t, editReplacedURL, storedHTTPConfig(t, env, tgt.ID).URL,
+	)
+}

internal/middleware/middleware_test.go

@@ -12,7 +12,6 @@ import (
 	"testing"
 	"time"
 
-	"github.com/gorilla/sessions"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"sneak.berlin/go/webhooker/internal/config"
@@ -78,14 +77,7 @@ func newTestSessionManager(
 		key[i] = byte(i)
 	}
 
-	store := sessions.NewCookieStore(key)
+	store := session.NewStore(key)
-	store.Options = &sessions.Options{
-		Path:     "/",
-		MaxAge:   86400 * 7,
-		HttpOnly: true,
-		Secure:   false,
-		SameSite: http.SameSiteLaxMode,
-	}
 
 	var now func() time.Time
 
@@ -931,8 +923,7 @@ func metricsAuthMiddleware(
 	}
 
 	key := make([]byte, testKeySize)
-	store := sessions.NewCookieStore(key)
+	store := session.NewStore(key)
-	store.Options = &sessions.Options{Path: "/", MaxAge: 86400}
 
 	sessManager := session.NewForTest(store, cfg, log, key, nil)
 

internal/session/export_test.go

@@ -1,10 +0,0 @@
-package session
-
-import "github.com/gorilla/sessions"
-
-// NewStore exposes the production cookie-store constructor so tests
-// exercise the store the application actually runs with, rather than a
-// lookalike assembled in the test.
-func NewStore(key []byte) *sessions.CookieStore {
-	return newStore(key)
-}

internal/session/testing.go

@@ -8,6 +8,13 @@ import (
 	"sneak.berlin/go/webhooker/internal/config"
 )
 
+// NewStore exposes the production cookie-store constructor so tests
+// exercise the store the application actually runs with, rather than a
+// lookalike assembled in the test.
+func NewStore(key []byte) *sessions.CookieStore {
+	return newStore(key)
+}
+
 // NewForTest creates a Session with a pre-configured cookie store for use
 // in tests. This bypasses the fx lifecycle and database dependency, allowing
 // middleware and handler tests to use real session functionality. The key