1 Commits

Author SHA1 Message Date
4dbec6757b Report handler panics through the logger and answer 500 (closes #187)
All checks were successful
check / check (push) Successful in 2m54s
chi v1.5.5's middleware.Recoverer neither logged a handler panic nor
answered 500. Its pretty-printer scans the stack for a frame beginning
"panic(0x", which the runtime no longer emits, so the scan never
terminates early and every line reaches decorateFuncCallLine, which
slices pkg[strings.Index(pkg, "."):] without checking for -1. That
second panic escaped chi's own deferred function, so its
WriteHeader(500) never ran: net/http closed the connection and reported
its own crash, losing the original panic value entirely.

Middleware.Recoverer replaces it. It writes one ERROR record through
internal/logger carrying the panic value, the stack and the request id,
and answers 500. http.ErrAbortHandler is re-panicked rather than
swallowed, and a response the handler already committed is left alone
rather than overwritten.

It is registered inside every middleware that observes the response, so
the 500 is the status the access log records and the metrics count, and
outside the sentryhttp handler, whose Repanic option needs something
further out to catch what it re-raises.

Every growable field on the record is bounded in encoded bytes, through
the same internal/logfield budget the access log spends: 512 for the
panic value, since a handler may build one out of the request, 128 for
the request id, which a client supplies outright through X-Request-Id,
and 8192 for the stack, cut at its far end so the panic site survives.
MaxPanicLogLineBytes states the resulting ceiling at 10240 over an
arithmetic sum of 9121. Driving all three past their budgets at once
measured 9009 bytes on the JSON handler and 8982 to 8983 on the text
one in one checkout, and the real case through the shipped chain
measures roughly 3960. Those figures are illustrations rather than
invariants: the stack's own content decides where its cut lands, and
debug.Stack() embeds absolute source paths, so both move. No test
asserts a figure; the tests assert the ceiling and that each growable
field was cut.

Because the panic record no longer reaches net/http's error log, the
carve-outs in README.md and in the MaxAccessLogLineBytes doc comment
that described that path are removed rather than reworded. What
replaces them states the ceiling the record is now written under, and
internal/server/recoverer_test.go asserts that "http: panic serving"
appears in neither of the process's streams.
2026-08-18 06:22:51 +00:00
3 changed files with 27 additions and 105 deletions

View File

@@ -1,6 +1,6 @@
--- ---
title: Repository Policies title: Repository Policies
last_modified: 2026-08-07 last_modified: 2026-07-06
--- ---
This document covers repository structure, tooling, and workflow standards. Code This document covers repository structure, tooling, and workflow standards. Code
@@ -189,13 +189,8 @@ style conventions are in separate documents:
module under test to verify it compiles/parses. There is no excuse for module under test to verify it compiles/parses. There is no excuse for
`make test` to be a no-op. `make test` to be a no-op.
- `make test` must complete in under 60 seconds. That is the hard cap, and a - `make test` must complete in under 20 seconds. Add a 30-second timeout in the
suite that exceeds it fails. Under 20 seconds is the target. A suite between Makefile.
20 and 60 seconds is still green, but the overage must be filed as an
improvement bug against that repo. Add a 90-second timeout to the test
invocation in the Makefile (`go test -timeout 90s`). The backstop deliberately
sits above the hard cap so that it catches a genuinely hung test rather than a
merely slow one.
- **`make test` should use the conditional verbose rerun pattern.** Run tests - **`make test` should use the conditional verbose rerun pattern.** Run tests
without `-v` (verbose) first. If tests fail, automatically rerun with `-v` to without `-v` (verbose) first. If tests fail, automatically rerun with `-v` to
@@ -214,9 +209,9 @@ style conventions are in separate documents:
```makefile ```makefile
test: test:
@go test -timeout 90s -race -cover ./... || \ @go test -timeout 30s -race -cover ./... || \
{ echo "--- Rerunning with -v for details ---"; \ { echo "--- Rerunning with -v for details ---"; \
go test -timeout 90s -race -v ./...; exit 1; } go test -timeout 30s -race -v ./...; exit 1; }
``` ```
Python example: Python example:
@@ -265,10 +260,7 @@ style conventions are in separate documents:
- `.golangci.yml` is standardized and must _NEVER_ be modified by an agent, only - `.golangci.yml` is standardized and must _NEVER_ be modified by an agent, only
manually by the user. Fetch from manually by the user. Fetch from
`https://git.eeqj.de/sneak/prompts/raw/branch/main/.golangci.yml`. The `https://git.eeqj.de/sneak/prompts/raw/branch/main/.golangci.yml`.
canonical golangci-lint version is v2.12.2 (released 2026-05-06), installed
commit-pinned via
`go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@c0d3ddc9cf3faa61a4e378e879ece580256d76e5`.
- When pinning images or packages by hash, add a comment above the reference - When pinning images or packages by hash, add a comment above the reference
with the version and date (YYYY-MM-DD). with the version and date (YYYY-MM-DD).

88
TODO.md
View File

@@ -24,84 +24,36 @@ event retention (#63), the database archiving target (#43), the admin
password change flow (#65), policy compliance (#6), pinned lint tooling password change flow (#65), policy compliance (#6), pinned lint tooling
(#55), and fail-loud configuration parsing (#80). (#55), and fail-loud configuration parsing (#80).
`next` holds the **complete 1.0.0 milestone**: every issue in it is `next` holds the 1.0.0 milestone less its final four issues (#176, #178,
closed, and it is verified green both by CI and by cache-defeated #186, #187 — all in review or held on merge order), and is verified
container runs (`docker build --no-cache-filter=lint green by cache-defeated container runs
--no-cache-filter=builder`). (`docker build --no-cache-filter=lint --no-cache-filter=builder`). The
CI status is not independently claimed here: a superseded run is
One caveat on reading a green check, narrower than it used to be. A recorded as `skipped` and still rolls up green, so a commit status on
docs-only commit deliberately replays from the layer cache (#119), so a `next` does not by itself evidence an executed check (#152). Before
green status on such a commit evidences a replay rather than an executed #119, a warm layer cache also let the gate report success without
run; a code commit invalidates the `COPY` layer and genuinely executes. executing anything, and replayed the previous build's console log so
Superseded runs are no longer the hazard they were: before #152 they the lie looked like a real run. Note: `TODO.md` was deliberately
were recorded as `skipped` and rolled up green, and before #119 a warm
layer cache let the gate report success without executing anything,
replaying the previous build's console log so the lie looked like a real
run. Both are fixed. Note: `TODO.md` was deliberately
deleted from this repo in f9a9569 (2026-03-01, #6); its content was deleted from this repo in f9a9569 (2026-03-01, #6); its content was
folded into the README TODO section, which this draft reconstructs as folded into the README TODO section, which this draft reconstructs as
of 2026-07-06. of 2026-07-06.
# Next Step # Next Step
Merge the milestone PR (#111) to `main` and tag 1.0.0 from it. The Land the last four 1.0.0 issues, then merge the milestone PR to `main`
milestone is empty and `next` is green; nothing else blocks the tag. and tag 1.0.0 from it. Merge order is forced by a real conflict on
`README.md` and `internal/middleware/middleware.go`: #186, then #176,
then #178, then #187.
Three items belong to the owner, none of them blocking. #150 was decided Two items belong to the owner, neither blocking the tag. #150 was
by the manager rather than left to stall the queue and is flagged on the decided by the manager rather than left to stall the queue and is
issue for reversal if that call was wrong. #112 (whether `Completed flagged on the issue for reversal if that call was wrong. #112 (whether
Steps` should exist at all, given it once conflicted on every unit) is `Completed Steps` should exist at all, given it once conflicted on every
unanswered; the provisional ruling in force is that issue branches do unit) is unanswered; the provisional ruling in force is that issue
not touch this file. #198 records that `make test` is past the org 20s branches do not touch this file.
target — 46s of test execution inside a 62.8s CI layer — and turns on
which quantity the 60s hard cap governs; it is scoped as the improvement
bug the 20-60s band requires, and should be milestoned instead if the
cap is read as covering the whole invocation.
After the tag, the largest open cluster is the unmilestoned follow-up
backlog these units generated: #183, #184, #185, #190, #191, #193 and
#198.
# Completed Steps # Completed Steps
- 2026-08-18 Raise `script/test`'s per-package timeout from 30s to 90s,
matching the org-wide backstop. `go test` applies `-timeout` per
package, and `internal/handlers` had grown past the old budget: a
cache-defeated build failed outright at `GOMAXPROCS=4`, and every run
under deliberate host load breached 30s. The measurement table lives
in the script (#194)
- 2026-08-18 Re-sync `REPO_POLICIES.md` from `prompts`. The local copy
was stale and still mandated a 20s test target with a 30s timeout,
which the org replaced with a 60s cap and a 90s backstop. A synced
copy is not a source; reading it as one nearly produced a PR against
`prompts` proposing a change already merged there (#196)
- 2026-08-18 Report handler panics through the logger and answer 500.
chi v1.5.5's `Recoverer` scans for a `panic(0x` frame the runtime no
longer emits, then indexes `pkg[-1:]`, so it panicked inside its own
stack printer before writing a byte: the recovery never ran, the
client got a dropped connection instead of a 500, and the original
panic was lost. A local middleware replaces it, bounded by
`MaxPanicLogLineBytes` (#187)
- 2026-08-18 Route GORM's logger through `slog` and bound it. Every
`gorm.Open` left `logger.Default` in place at `Warn` with
`IgnoreRecordNotFoundError` false, so **every record-not-found
printed the fully interpolated SQL to stdout** — including the
client-chosen path on `/webhook/{uuid}` and the submitted username on
the login form, at no level the operator set and outside
`internal/logger` entirely. Three call sites, not the two the issue
named (#178)
- 2026-08-18 Bound every `slog` line against client-chosen text. Eight
sites reachable unauthenticated, found by reading every `slog` call in
the tree rather than only the one reported; the budget moved to a
shared `internal/logfield` so no second truncation exists. `DEBUG`
being off by default is not a bound and is not treated as one (#176)
- 2026-08-18 Stop a slow host turning a login-guard test into a
segfault. A non-fatal `assert` on an acquire result was dereferenced
on the next line, so one timing miss killed the whole
`internal/middleware` binary and reddened CI for unrelated PRs. The
fix also removed a real production race — `acquire` could shed a
request with a slot standing free, because Go picks uniformly among
ready `select` cases (#186)
- 2026-08-18 Send the chi route pattern to Sentry rather than the - 2026-08-18 Send the chi route pattern to Sentry rather than the
concrete path. The receiver's path carries the entrypoint capability concrete path. The receiver's path carries the entrypoint capability
token, so every Sentry event from `/webhook/{uuid}` shipped a live token, so every Sentry event from `/webhook/{uuid}` shipped a live

View File

@@ -1,34 +1,12 @@
#!/bin/sh #!/bin/sh
# script/test: run the test suite. # script/test: run the test suite.
#
# -timeout is applied by `go test` per package, not to the run as a whole, so
# it only has to clear the slowest single package. That is internal/handlers,
# measured in a cache-defeated builder stage on the 48-core shared build host
# (2026-08-18); load- and host-dependent, not invariants:
#
# 16.9s host load 5-20, GOMAXPROCS 48
# 45.9s / 47.3s / 49.0s three runs at deliberate host load 31-73
# 30.6s / 39.7s host load 5-20, GOMAXPROCS 6 / 4
# 67.3s / 97.5s host load 5-20, GOMAXPROCS 2 / 1
# 67.3s GOMAXPROCS 4 at deliberate host load 52-68
#
# The old 30s budget was breached by every loaded run and by every GOMAXPROCS
# at or below 6; at GOMAXPROCS 4 it failed outright ("panic: test timed out
# after 30s"), reproduced on 33e4fa4 with no other change.
#
# 90s matches the org-wide backstop in REPO_POLICIES.md and is sized here
# against the figures above: the worst case under native parallelism is 49.0s,
# and the compound GOMAXPROCS-4-under-load case at 67.3s sits at 75% of it.
# The one figure above 90s is GOMAXPROCS 1, a synthetic core floor rather than
# a condition CI runs under. If a CPU-limited runner ever puts a real run near
# 67s, that is the datum to revisit the org figure with.
set -eu set -eu
ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" ROOT="$(cd "$(dirname "$0")/.." && pwd -P)"
main() { main() {
cd "$ROOT" cd "$ROOT"
go test -v -race -timeout 90s ./... go test -v -race -timeout 30s ./...
} }
main "$@" main "$@"