sneak/webhooker
1
0
Fork 0
Code Issues 34 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Serve an event's full stored body over HTTP (closes #157) #167

Merged
clawbot merged 1 commits from issue-157-event-body-download into next 2026-08-18 00:41:32 +02:00
Conversation 3 Commits 1 Files Changed 7 +911 -28
clawbot commented 2026-08-17 23:18:15 +02:00
Collaborator
Copy Link

Closes #157.

GET /source/{sourceID}/logs/{eventID}/body serves one event's whole stored body. The event log's truncation marker links to it, and only when a body was actually cut.

What was missing

The 8 KB render cap left storage untouched but no route served the rest, so a body over the cap was reachable only with filesystem access to the SQLite files. The route closes that.

The two load-bearing constraints

Not renderable. application/octet-stream, Content-Disposition: attachment, X-Content-Type-Options: nosniff. The bytes are chosen by whoever can reach the public receiver and are handed back inside the operator's authenticated origin, so this is a stored-XSS sink if served as anything a browser will parse.

I checked rather than assumed on the other two points:

  • SecurityHeaders() is a global router middleware and does set nosniff on every response (internal/middleware/middleware.go:269). The handler sets it again anyway, so the guarantee belongs to the route rather than to a middleware someone could reorder or scope away, and so the handler test can assert it directly.
  • The existing CSP does not cover this path. It is default-src 'self'; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline'. A document served from this origin is 'self', and inline script is explicitly allowed, so CSP would not stop a stored HTML payload executing. The disposition is the control, not a backstop to CSP.

The filename cannot be steered: the eventID param goes through uuid.Parse before anything else, and the header is built from the parsed value's canonical form, which is a fixed alphabet. A non-uuid id is a 404 that never reaches SQL or a header.

Bounded — at roughly two bodies per download, not one. The body is read in a single query and held whole while it is written to the client. There is no cheaper read available:

  • modernc.org/sqlite reaches the app through database/sql, which exposes no incremental handle on a SQLite BLOB (sqlite3_blob_open has no database/sql surface, conn is unexported), so there is no row stream to take.
  • Reading byte ranges with substr(cast(body as blob), ?, ?) does not avoid the cost either. SQLite materialises the entire column value to evaluate each substr call, so range reads pay for the whole body once per range instead of once per download. An earlier revision of this PR chunked at 64 KiB and claimed a chunk-sized bound; that claim was wrong and is gone from the code, the commit message and this body.

The cost, corrected from the previous revision of this body: the route is owner-authenticated and ingest is capped at 1 MB, so a download is bounded — but at roughly two body-sized allocations, not one. Two copies are live at the same time during the write: the driver's column buffer, and the clone database/sql makes in convertAssign when a []byte column is scanned into a *[]byte. Measured Go-heap allocation per download, production handler with the ResponseWriter discarded, mean of 10 runs:

body=65536     alloc=175850    2.68x
body=262144    alloc=574384    2.19x
body=524288    alloc=1102961   2.10x
body=1048576   alloc=2156499   2.06x

alloc ~= 2 x body + ~45 KB, linear — so ~2 MB of Go heap at the ingest cap, not 1 MB. SQLite's own materialisation of the column value lives in modernc's allocator outside the Go heap and is therefore not in those numbers, so real process peak is higher again. Treat 2x as a floor, not a hard process-peak ceiling. The doc comment on serveEventBody and the commit message state the same thing.

Nothing goes through renderTemplate. Content-Length comes from the length of the bytes actually read, so it cannot disagree with what is written.

Measured, 1 MiB body, 5 downloads each, same test harness, -race on, two runs:

chunked 64 KiB: 612.86ms / 582.97ms   single read: 54.26ms / 38.28ms   (11.3x / 15.2x)

Two consequences of the single read beyond speed. There is no read transaction spanning the download, so no read lock is held while a slow client drains — these per-webhook databases run in SQLite's default journal mode rather than WAL, and a lock held that long would block the receiver from recording new events. And because the body is read before the first header is written, an event reaped mid-request cannot tear a response: it is either served whole or 404s cleanly. The previous revision's errShortBodyRead was unreachable dead code (a reaped row yields sql.ErrNoRows from the scan, never an empty chunk); it is deleted, and both deletion paths the codebase performs are now pinned by a test.

Accepted deviation from the definition of done

#157's definition of done says the route "streams from the row rather than buffering it". This change buffers the whole body instead. That is a knowing deviation, accepted rather than overlooked: database/sql exposes no incremental BLOB handle, so no streaming path exists to take — two independent reviewers have now confirmed that, and the substr range-read alternative is slower and does not lower the peak. The same DoD sentence sanctions the 1 MB ingest cap as the bound, which is what the route relies on, at the ~2x cost measured above.

Authorization

The id = ? AND user_id = ? lookup that the log page applies is extracted as ownedWebhook in internal/handlers/source_management.go and shared by HandleSourceLogs and the new handler, so the download cannot come to authorize differently from the page that links to it. Ownership and existence are one query: another user's webhook and a nonexistent one are the same 404.

Scope note: the same two-line lookup is still hand-written in about eight other handlers in that file. I did not convert them — that is unrelated to this issue and would widen the diff across handlers this change does not touch.

An honest correction on the IDOR guard

Every event query is scoped id = ? AND webhook_id = ?, but that predicate is not what makes an event id from a sibling webhook a 404. Events live in a per-webhook SQLite file, so the sibling's event is not in the database being queried at all. I confirmed this by mutation: replacing the webhook_id predicate with a tautology leaves the cross-webhook test green. The predicate stays as a second guard that survives any future change putting more than one webhook's events in one file, and both the handler doc comment and the test say which mechanism is load-bearing.

Tests

internal/handlers/event_body_test.go: a 200 KiB body with multibyte runes comes back byte-identical with a matching Content-Length; a table of sizes — empty, one byte, one below the render cap, exactly the cap, one above, and a body with NUL bytes, invalid UTF-8 and a multibyte rune — all round-trip byte-identical with Content-Length equal to bytes written; a reaped event 404s with no Content-Length and no Content-Disposition, covering both the soft delete and the hard delete the reaper performs; another user's event 404s and its payload does not appear; an event id from a sibling webhook 404s; disposition, octet-stream type and nosniff are pinned; a <script> body is returned unaltered but as an attachment of opaque bytes; unknown, non-uuid and injection-shaped ids 404 without reaching a header.

internal/server/routes_test.go closes the coverage gap the review found: TestSourceLogs_TruncationLinkDownloadsTheBody renders the log page through the production router, takes the download URL out of the markup the template emitted, and fetches that URL through the router again, asserting the bytes and every header. Nothing in it is hand-written. TestSourceLogsBody_OtherUser404s drives the same real URL as another logged-in user (404) and unauthenticated (303 to /pages/login).

Mutation evidence

Route pattern /logs/{eventID}/body → /logs/{eventID}/bodyy:

--- FAIL: TestSourceLogs_TruncationLinkDownloadsTheBody (0.60s)
ok  	sneak.berlin/go/webhooker/internal/handlers	(cached)

The handler package stays green — which is exactly the hole the new test fills. Template href /source/… → /sources/… fails both TestSourceLogs_TruncationLinkDownloadsTheBody and TestHandleSourceLogs_TruncationMarkerLinksToDownload. Both mutations were reverted.

The ownership mutation from the previous revision still stands unchanged (ownedWebhook is untouched by this rework): dropping AND user_id = ? serves the other user's payload with a 200 and fails TestHandleEventBodyDownload_OtherUsersEvent404s.

This revision

Text only. The whole diff against the previous head 6f37d05 is the serveEventBody doc comment plus the commit message: no code, no tests, no design change. git range-diff over the two commits shows comment and message hunks and nothing else.

Gate

Both gates re-run on the pushed commit, rebased onto next at bef9986.

make check exits 0 with pristine GOCACHE and GOLANGCI_LINT_CACHE: all 12 packages ran with real durations, zero (cached), 0 issues., fmt-check clean.

docker build --no-cache-filter=lint --no-cache-filter=builder . exits 0:

#18 [lint 8/8] RUN make lint
#18 52.53 0 issues.
#18 DONE 52.9s
#26 [builder  9/11] RUN make test
#26 DONE 55.9s

Real per-package durations, zero (cached) lines anywhere in the build log, neither stage CACHED:

ok  	sneak.berlin/go/webhooker/internal/handlers	6.622s
ok  	sneak.berlin/go/webhooker/internal/server	3.164s
ok  	sneak.berlin/go/webhooker/internal/database	3.712s

Every tagged image was removed and docker ps -a shows nothing of mine. No prune of any kind was run.

Unrelated observation, not addressed here: the pinned linter config still enables gomodguard, which v2.12.0 deprecated in favour of gomodguard_v2. It warns on every run and is repo-wide, not this route's business.

TODO.md is untouched.

Closes https://git.eeqj.de/sneak/webhooker/issues/157. `GET /source/{sourceID}/logs/{eventID}/body` serves one event's whole stored body. The event log's truncation marker links to it, and only when a body was actually cut. ## What was missing The 8 KB render cap left storage untouched but no route served the rest, so a body over the cap was reachable only with filesystem access to the SQLite files. The route closes that. ## The two load-bearing constraints **Not renderable.** `application/octet-stream`, `Content-Disposition: attachment`, `X-Content-Type-Options: nosniff`. The bytes are chosen by whoever can reach the public receiver and are handed back inside the operator's authenticated origin, so this is a stored-XSS sink if served as anything a browser will parse. I checked rather than assumed on the other two points: - `SecurityHeaders()` is a global router middleware and does set `nosniff` on every response (`internal/middleware/middleware.go:269`). The handler sets it again anyway, so the guarantee belongs to the route rather than to a middleware someone could reorder or scope away, and so the handler test can assert it directly. - **The existing CSP does not cover this path.** It is `default-src 'self'; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline'`. A document served from this origin is `'self'`, and inline script is explicitly allowed, so CSP would not stop a stored HTML payload executing. The disposition is the control, not a backstop to CSP. The filename cannot be steered: the `eventID` param goes through `uuid.Parse` before anything else, and the header is built from the parsed value's canonical form, which is a fixed alphabet. A non-uuid id is a 404 that never reaches SQL or a header. **Bounded — at roughly two bodies per download, not one.** The body is read in a single query and held whole while it is written to the client. There is no cheaper read available: - `modernc.org/sqlite` reaches the app through `database/sql`, which exposes no incremental handle on a SQLite BLOB (`sqlite3_blob_open` has no `database/sql` surface, `conn` is unexported), so there is no row stream to take. - Reading byte ranges with `substr(cast(body as blob), ?, ?)` does not avoid the cost either. SQLite materialises the **entire** column value to evaluate each `substr` call, so range reads pay for the whole body once per range instead of once per download. An earlier revision of this PR chunked at 64 KiB and claimed a chunk-sized bound; that claim was wrong and is gone from the code, the commit message and this body. The cost, corrected from the previous revision of this body: the route is owner-authenticated and ingest is capped at 1 MB, so a download is bounded — but at **roughly two body-sized allocations, not one**. Two copies are live at the same time during the write: the driver's column buffer, and the clone `database/sql` makes in `convertAssign` when a `[]byte` column is scanned into a `*[]byte`. Measured Go-heap allocation per download, production handler with the `ResponseWriter` discarded, mean of 10 runs: ``` body=65536 alloc=175850 2.68x body=262144 alloc=574384 2.19x body=524288 alloc=1102961 2.10x body=1048576 alloc=2156499 2.06x ``` `alloc ~= 2 x body + ~45 KB`, linear — so ~2 MB of Go heap at the ingest cap, not 1 MB. SQLite's own materialisation of the column value lives in modernc's allocator **outside** the Go heap and is therefore not in those numbers, so real process peak is higher again. Treat 2x as a floor, not a hard process-peak ceiling. The doc comment on `serveEventBody` and the commit message state the same thing. Nothing goes through `renderTemplate`. `Content-Length` comes from the length of the bytes actually read, so it cannot disagree with what is written. Measured, 1 MiB body, 5 downloads each, same test harness, `-race` on, two runs: ``` chunked 64 KiB: 612.86ms / 582.97ms single read: 54.26ms / 38.28ms (11.3x / 15.2x) ``` Two consequences of the single read beyond speed. There is no read transaction spanning the download, so no read lock is held while a slow client drains — these per-webhook databases run in SQLite's default journal mode rather than WAL, and a lock held that long would block the receiver from recording new events. And because the body is read before the first header is written, an event reaped mid-request cannot tear a response: it is either served whole or 404s cleanly. The previous revision's `errShortBodyRead` was unreachable dead code (a reaped row yields `sql.ErrNoRows` from the scan, never an empty chunk); it is deleted, and both deletion paths the codebase performs are now pinned by a test. ## Accepted deviation from the definition of done https://git.eeqj.de/sneak/webhooker/issues/157's definition of done says the route "streams from the row rather than buffering it". **This change buffers the whole body instead.** That is a knowing deviation, accepted rather than overlooked: `database/sql` exposes no incremental BLOB handle, so no streaming path exists to take — two independent reviewers have now confirmed that, and the `substr` range-read alternative is slower and does not lower the peak. The same DoD sentence sanctions the 1 MB ingest cap as the bound, which is what the route relies on, at the ~2x cost measured above. ## Authorization The `id = ? AND user_id = ?` lookup that the log page applies is extracted as `ownedWebhook` in `internal/handlers/source_management.go` and shared by `HandleSourceLogs` and the new handler, so the download cannot come to authorize differently from the page that links to it. Ownership and existence are one query: another user's webhook and a nonexistent one are the same 404. Scope note: the same two-line lookup is still hand-written in about eight other handlers in that file. I did not convert them — that is unrelated to this issue and would widen the diff across handlers this change does not touch. ## An honest correction on the IDOR guard Every event query is scoped `id = ? AND webhook_id = ?`, but that predicate is **not** what makes an event id from a sibling webhook a 404. Events live in a per-webhook SQLite file, so the sibling's event is not in the database being queried at all. I confirmed this by mutation: replacing the `webhook_id` predicate with a tautology leaves the cross-webhook test green. The predicate stays as a second guard that survives any future change putting more than one webhook's events in one file, and both the handler doc comment and the test say which mechanism is load-bearing. ## Tests `internal/handlers/event_body_test.go`: a 200 KiB body with multibyte runes comes back byte-identical with a matching `Content-Length`; a table of sizes — empty, one byte, one below the render cap, exactly the cap, one above, and a body with NUL bytes, invalid UTF-8 and a multibyte rune — all round-trip byte-identical with `Content-Length` equal to bytes written; a reaped event 404s with no `Content-Length` and no `Content-Disposition`, covering both the soft delete and the hard delete the reaper performs; another user's event 404s and its payload does not appear; an event id from a sibling webhook 404s; disposition, octet-stream type and nosniff are pinned; a `<script>` body is returned unaltered but as an attachment of opaque bytes; unknown, non-uuid and injection-shaped ids 404 without reaching a header. `internal/server/routes_test.go` closes the coverage gap the review found: `TestSourceLogs_TruncationLinkDownloadsTheBody` renders the log page through the production router, takes the download URL out of the markup the template emitted, and fetches that URL through the router again, asserting the bytes and every header. Nothing in it is hand-written. `TestSourceLogsBody_OtherUser404s` drives the same real URL as another logged-in user (404) and unauthenticated (303 to `/pages/login`). ## Mutation evidence Route pattern `/logs/{eventID}/body` → `/logs/{eventID}/bodyy`: ``` --- FAIL: TestSourceLogs_TruncationLinkDownloadsTheBody (0.60s) ok sneak.berlin/go/webhooker/internal/handlers (cached) ``` The handler package stays green — which is exactly the hole the new test fills. Template `href` `/source/…` → `/sources/…` fails both `TestSourceLogs_TruncationLinkDownloadsTheBody` and `TestHandleSourceLogs_TruncationMarkerLinksToDownload`. Both mutations were reverted. The ownership mutation from the previous revision still stands unchanged (`ownedWebhook` is untouched by this rework): dropping `AND user_id = ?` serves the other user's payload with a 200 and fails `TestHandleEventBodyDownload_OtherUsersEvent404s`. ## This revision Text only. The whole diff against the previous head `6f37d05` is the `serveEventBody` doc comment plus the commit message: no code, no tests, no design change. `git range-diff` over the two commits shows comment and message hunks and nothing else. ## Gate Both gates re-run on the pushed commit, rebased onto `next` at `bef9986`. `make check` exits 0 with pristine `GOCACHE` and `GOLANGCI_LINT_CACHE`: all 12 packages ran with real durations, **zero** `(cached)`, `0 issues.`, fmt-check clean. `docker build --no-cache-filter=lint --no-cache-filter=builder .` exits 0: ``` #18 [lint 8/8] RUN make lint #18 52.53 0 issues. #18 DONE 52.9s #26 [builder 9/11] RUN make test #26 DONE 55.9s ``` Real per-package durations, zero `(cached)` lines anywhere in the build log, neither stage `CACHED`: ``` ok sneak.berlin/go/webhooker/internal/handlers 6.622s ok sneak.berlin/go/webhooker/internal/server 3.164s ok sneak.berlin/go/webhooker/internal/database 3.712s ``` Every tagged image was removed and `docker ps -a` shows nothing of mine. No prune of any kind was run. Unrelated observation, not addressed here: the pinned linter config still enables `gomodguard`, which v2.12.0 deprecated in favour of `gomodguard_v2`. It warns on every run and is repo-wide, not this route's business. `TODO.md` is untouched.
clawbot added the needs-review label 2026-08-17 23:18:15 +02:00
clawbot added 1 commit 2026-08-17 23:18:15 +02:00
Serve an event's full stored body over HTTP (closes #157)
All checks were successful
check / check (push) Successful in 3m9s
Details
be9e13eea9 
Capping the event log page at 8 KB of body per event left no
in-app way to see a larger one: storage keeps it, but no route
served it, so a payload over the cap was reachable only by an
operator with filesystem access. GitHub pull_request and
multi-commit push payloads, expanded Stripe events and Shopify
orders all routinely clear 8 KB, which is exactly when the tool
is supposed to be useful.

GET /source/{sourceID}/logs/{eventID}/body now serves one whole
body, and the truncation marker links to it when — and only
when — a body was actually cut.

The response is deliberately inert. Its bytes are chosen by
whoever can reach the public receiver and it hands them back
inside the operator's own authenticated origin, so it goes out
as application/octet-stream with Content-Disposition: attachment
and nosniff, and the filename is built from a parsed uuid rather
than from anything in the request. The application CSP is no
help on this path: script-src allows 'unsafe-inline' from
'self', so a document served from this origin could run its own
script.

It is also bounded. database/sql exposes no incremental handle
on a SQLite blob, so scanning the column would materialise the
whole body whatever wraps it; instead the body is read in 64 KiB
ranges with substr over a blob cast and each range is written
straight to the ResponseWriter, never through renderTemplate.
Peak resident body bytes is the chunk, not the payload, which is
the memory profile the render cap was introduced to protect.

The ownership check the log page applies is extracted as
ownedWebhook and shared with the download, so the two cannot
drift apart. A webhook owned by someone else and one that does
not exist are the same 404.
clawbot self-assigned this 2026-08-17 23:18:19 +02:00
clawbot commented 2026-08-17 23:33:55 +02:00
Author
Collaborator
Copy Link

FAIL — needs-rework

Reviewed at be9e13e in an independent clone. Constraint 1 (non-renderable response) holds and is verified on a real response through the production router. Constraint 2 does not hold as claimed.

Finding 1 — the memory bound the design rests on is not real (internal/handlers/event_body.go:14-21, :195-212)

writeEventBody's doc comment, the commit message and the PR body all state "peak resident body bytes is the chunk, not the payload". That is true of the Go heap only. Each substr(cast(body as blob), ?, ?) call makes SQLite materialise the entire column value before slicing it, so the process holds the whole body — once per chunk query, N times per download. The whole body is buffered; it is just buffered in SQLite's allocator instead of Go's.

Measured, not inferred. Holding chunk size at 1 KiB and varying body size, per-chunk-query cost scales with body size rather than with chunk size:

PERQUERY size=16384    queries=16    elapsed=12.07ms   per_query=754us
PERQUERY size=262144   queries=256   elapsed=298.5ms   per_query=1.166ms
PERQUERY size=1048576  queries=1024  elapsed=2.630s    per_query=2.568ms

A constant-cost point query would give a flat per_query. It grows ~linearly in body size, which is the full-value materialisation showing through. Same body, chunk size varied:

bodyChunkBytes = 64 KiB : 1 MiB x5 = 301ms
bodyChunkBytes =  1 KiB : 1 MiB x5 = 15.99s

53x slower for 64x more queries over identical bytes — cost tracks query count, not bytes moved. Consequences at the shipped 64 KiB:

  • Process peak for a 1 MiB body is ~1 MiB (the ingest cap), not 64 KiB. The stated bound overstates by ~16x.
  • Serving 1 MiB costs 16 full-body materialisations, ~16x the CPU of a single scan, for a peak that a single scan would put at ~2 MiB rather than ~1 MiB.

Why it matters beyond accuracy: the no-transaction trade, the torn-response failure mode and the extra query path are all justified by this bound in the code comments. A reader maintaining this route will believe a property the code does not have.

What acceptable looks like: either state the true bound (Go-heap resident is one chunk; SQLite still materialises the whole value per query, so process peak is the ingest cap) and justify keeping 16 queries against 1, or drop the chunking for a single read and say the bound is the 1 MB ingest cap — which is what #157's definition of done already allows ("the 1 MB ingest cap is the only bound needed"). Whichever is chosen, the doc comments, the commit message and the PR body must agree with it.

The database/sql disclosure itself checks out: modernc.org/sqlite v1.28.0 exports no blob handle, conn is unexported and has no sqlite3_blob_open surface, so there is no streaming path to take even via sql.Conn.Raw. Nothing in this finding contradicts that — the objection is to the bound claimed for the fallback, not to the fallback existing.

Finding 2 — route registration is untested (internal/server/routes.go:154)

Every handler test builds its own chi.RouteContext and calls HandleEventBodyDownload() directly, and the template test asserts a hand-written URL string. A typo in either the route pattern or the template href leaves the whole suite green with the feature unreachable. internal/server/routes_test.go already has newTestEnv driving the production route tree; one GET through it would close this. I wrote that test locally and the route does work end to end, so this is a coverage gap, not a live break.

Finding 3 — errShortBodyRead is unreachable (internal/handlers/event_body.go:37-40, :236-238)

If the row is reaped mid-download, Row().Scan returns sql.ErrNoRows, not an empty chunk, so the len(chunk) == 0 branch cannot fire for the case its comment describes. Events are immutable once written and the reaper hard-deletes (internal/database/retention.go:310), so no path shortens a body in place. Either drop the branch or reduce it to a defensive guard whose comment does not claim to be the reaped-mid-download detector. The reaped-mid-download path is also untested; given it is a deliberate accepted failure mode, it is worth pinning that the response ends short and the error is logged.

Verified and passing

Ownership and isolation, both constraints' observable behaviour, boundaries, and both gates.

  • Real response through the production router: Content-Type: application/octet-stream, Content-Disposition: attachment; filename="webhooker-event-<uuid>.bin", X-Content-Type-Options: nosniff, Content-Length exact, bytes byte-identical, Cache-Control: no-store. A <script>alert(document.cookie)</script> payload posted to the public receiver came back inert as an attachment. CSP on the response is default-src 'self'; script-src 'self' 'unsafe-inline' — confirms the PR's point that CSP is no backstop here and disposition is the control. nosniff comes from both the handler and global SecurityHeaders.
  • Filename is not steerable: ../../etc/passwd, x"; rm -rf / and not-a-uuid all 404 with no Content-Disposition emitted.
  • Ownership mutation re-run: dropping AND user_id = ? from ownedWebhook fails TestHandleEventBodyDownload_OtherUsersEvent404s with 200 and the other user's payload in the body — matches the reported output exactly.
  • webhook_id mutation re-run: replacing both predicates with a tautology leaves the suite green. The stated explanation is correct, and the per-webhook file boundary is reliable isolation here — GetDB is keyed on webhook.ID taken from the row ownedWebhook returned, dbPath derives the filename from that same id, and the sync.Map cache is keyed identically, so no path in the code opens one webhook's file under another's id. Unauthenticated 303 to /pages/login; another logged-in user 404s; both misses are the same query path, so no timing or message leak.
  • Chunk boundaries, run directly: sizes 0, 1, 64Ki-1, 64Ki, 64Ki+1, 128Ki and 1 MiB+1 all return byte-identical with Content-Length equal to bytes written. A body with NUL bytes, invalid UTF-8 and a multibyte rune straddling the first chunk boundary round-trips unchanged. A soft-deleted event 404s with no Content-Length emitted, so there is no torn response from that direction.
  • make check exit 0 with isolated GOLANGCI_LINT_CACHE, 0 issues., zero (cached) test packages. docker build --no-cache-filter=lint --no-cache-filter=builder . exit 0: #21 [lint 8/8] RUN make lint 57.1s 0 issues., #34 [builder 9/11] RUN make test 60.7s, real per-package durations, zero (cached). No prune run; image removed, docker ps -a clear of anything of mine.
  • CI green on be9e13e (check / check, 3m9s). Mergeable against next, one commit, title ends (closes #157), TODO.md untouched, no Claude/Anthropic references or attribution trailers, no non-inclusive terminology, make fmt-check clean.

Notes, not findings

  • HEAD on the download path returns 405. Every other GET route in the repo behaves the same way, so this is the existing chi convention rather than something this PR introduced.
  • Asset trap confirmed and worked around by running make assets before the gate; the two vendored-asset tests pass afterwards.
  • Disclosure: the boundary, timing and end-to-end probes above were run as temporary test files invoked with a filtered go test because make test cannot select a subset. The two authoritative gates were run only through make check and the Docker build. All probe files were deleted and both clones left clean.
FAIL — needs-rework Reviewed at `be9e13e` in an independent clone. Constraint 1 (non-renderable response) holds and is verified on a real response through the production router. Constraint 2 does not hold as claimed. ## Finding 1 — the memory bound the design rests on is not real (`internal/handlers/event_body.go:14-21`, `:195-212`) `writeEventBody`'s doc comment, the commit message and the PR body all state "peak resident body bytes is the chunk, not the payload". That is true of the Go heap only. Each `substr(cast(body as blob), ?, ?)` call makes SQLite materialise the **entire** column value before slicing it, so the process holds the whole body — once per chunk query, N times per download. The whole body is buffered; it is just buffered in SQLite's allocator instead of Go's. Measured, not inferred. Holding chunk size at 1 KiB and varying body size, per-chunk-query cost scales with body size rather than with chunk size: ``` PERQUERY size=16384 queries=16 elapsed=12.07ms per_query=754us PERQUERY size=262144 queries=256 elapsed=298.5ms per_query=1.166ms PERQUERY size=1048576 queries=1024 elapsed=2.630s per_query=2.568ms ``` A constant-cost point query would give a flat `per_query`. It grows ~linearly in body size, which is the full-value materialisation showing through. Same body, chunk size varied: ``` bodyChunkBytes = 64 KiB : 1 MiB x5 = 301ms bodyChunkBytes = 1 KiB : 1 MiB x5 = 15.99s ``` 53x slower for 64x more queries over identical bytes — cost tracks query count, not bytes moved. Consequences at the shipped 64 KiB: - Process peak for a 1 MiB body is ~1 MiB (the ingest cap), not 64 KiB. The stated bound overstates by ~16x. - Serving 1 MiB costs 16 full-body materialisations, ~16x the CPU of a single scan, for a peak that a single scan would put at ~2 MiB rather than ~1 MiB. Why it matters beyond accuracy: the no-transaction trade, the torn-response failure mode and the extra query path are all justified by this bound in the code comments. A reader maintaining this route will believe a property the code does not have. What acceptable looks like: either state the true bound (Go-heap resident is one chunk; SQLite still materialises the whole value per query, so process peak is the ingest cap) and justify keeping 16 queries against 1, or drop the chunking for a single read and say the bound is the 1 MB ingest cap — which is what https://git.eeqj.de/sneak/webhooker/issues/157's definition of done already allows ("the 1 MB ingest cap is the only bound needed"). Whichever is chosen, the doc comments, the commit message and the PR body must agree with it. The `database/sql` disclosure itself checks out: `modernc.org/sqlite v1.28.0` exports no blob handle, `conn` is unexported and has no `sqlite3_blob_open` surface, so there is no streaming path to take even via `sql.Conn.Raw`. Nothing in this finding contradicts that — the objection is to the bound claimed for the fallback, not to the fallback existing. ## Finding 2 — route registration is untested (`internal/server/routes.go:154`) Every handler test builds its own `chi.RouteContext` and calls `HandleEventBodyDownload()` directly, and the template test asserts a hand-written URL string. A typo in either the route pattern or the template `href` leaves the whole suite green with the feature unreachable. `internal/server/routes_test.go` already has `newTestEnv` driving the production route tree; one GET through it would close this. I wrote that test locally and the route does work end to end, so this is a coverage gap, not a live break. ## Finding 3 — `errShortBodyRead` is unreachable (`internal/handlers/event_body.go:37-40`, `:236-238`) If the row is reaped mid-download, `Row().Scan` returns `sql.ErrNoRows`, not an empty chunk, so the `len(chunk) == 0` branch cannot fire for the case its comment describes. Events are immutable once written and the reaper hard-deletes (`internal/database/retention.go:310`), so no path shortens a body in place. Either drop the branch or reduce it to a defensive guard whose comment does not claim to be the reaped-mid-download detector. The reaped-mid-download path is also untested; given it is a deliberate accepted failure mode, it is worth pinning that the response ends short and the error is logged. ## Verified and passing Ownership and isolation, both constraints' observable behaviour, boundaries, and both gates. - Real response through the production router: `Content-Type: application/octet-stream`, `Content-Disposition: attachment; filename="webhooker-event-<uuid>.bin"`, `X-Content-Type-Options: nosniff`, `Content-Length` exact, bytes byte-identical, `Cache-Control: no-store`. A `<script>alert(document.cookie)</script>` payload posted to the public receiver came back inert as an attachment. CSP on the response is `default-src 'self'; script-src 'self' 'unsafe-inline'` — confirms the PR's point that CSP is no backstop here and disposition is the control. `nosniff` comes from both the handler and global `SecurityHeaders`. - Filename is not steerable: `../../etc/passwd`, `x"; rm -rf /` and `not-a-uuid` all 404 with no `Content-Disposition` emitted. - Ownership mutation re-run: dropping `AND user_id = ?` from `ownedWebhook` fails `TestHandleEventBodyDownload_OtherUsersEvent404s` with 200 and the other user's payload in the body — matches the reported output exactly. - `webhook_id` mutation re-run: replacing both predicates with a tautology leaves the suite green. The stated explanation is correct, and the per-webhook file boundary is reliable isolation here — `GetDB` is keyed on `webhook.ID` taken from the row `ownedWebhook` returned, `dbPath` derives the filename from that same id, and the `sync.Map` cache is keyed identically, so no path in the code opens one webhook's file under another's id. Unauthenticated 303 to `/pages/login`; another logged-in user 404s; both misses are the same query path, so no timing or message leak. - Chunk boundaries, run directly: sizes 0, 1, 64Ki-1, 64Ki, 64Ki+1, 128Ki and 1 MiB+1 all return byte-identical with `Content-Length` equal to bytes written. A body with NUL bytes, invalid UTF-8 and a multibyte rune straddling the first chunk boundary round-trips unchanged. A soft-deleted event 404s with no `Content-Length` emitted, so there is no torn response from that direction. - `make check` exit 0 with isolated `GOLANGCI_LINT_CACHE`, `0 issues.`, zero `(cached)` test packages. `docker build --no-cache-filter=lint --no-cache-filter=builder .` exit 0: `#21 [lint 8/8] RUN make lint` 57.1s `0 issues.`, `#34 [builder 9/11] RUN make test` 60.7s, real per-package durations, zero `(cached)`. No prune run; image removed, `docker ps -a` clear of anything of mine. - CI green on `be9e13e` (`check / check`, 3m9s). Mergeable against `next`, one commit, title ends ` (closes #157)`, `TODO.md` untouched, no Claude/Anthropic references or attribution trailers, no non-inclusive terminology, `make fmt-check` clean. ## Notes, not findings - HEAD on the download path returns 405. Every other GET route in the repo behaves the same way, so this is the existing `chi` convention rather than something this PR introduced. - Asset trap confirmed and worked around by running `make assets` before the gate; the two vendored-asset tests pass afterwards. - Disclosure: the boundary, timing and end-to-end probes above were run as temporary test files invoked with a filtered `go test` because `make test` cannot select a subset. The two authoritative gates were run only through `make check` and the Docker build. All probe files were deleted and both clones left clean.
clawbot added needs-rework and removed needs-review labels 2026-08-17 23:35:15 +02:00
clawbot force-pushed issue-157-event-body-download from be9e13eea9 to 1ec8856bce 2026-08-17 23:48:26 +02:00 Compare
clawbot force-pushed issue-157-event-body-download from 1ec8856bce to b1cf0de216 2026-08-17 23:53:33 +02:00 Compare
clawbot force-pushed issue-157-event-body-download from b1cf0de216 to 6f37d05ab6 2026-08-17 23:58:12 +02:00 Compare
clawbot added needs-review and removed needs-rework labels 2026-08-17 23:59:30 +02:00
clawbot commented 2026-08-18 00:11:08 +02:00
Author
Collaborator
Copy Link

FAIL — needs-rework

Reviewed at 6f37d05 in an independent clone. Findings 2 and 3 of
#167 (comment) are closed and
I re-ran both mutations to prove it. Finding 1 is closed in substance — the
chunking is gone and no chunk/streaming/peak-is-one-chunk claim survives in the
code, the doc comments, the commit message or the PR body — but the bound that
replaced it is still overstated, by 2x instead of 16x.

Finding — "peak is one body per concurrent download" is measurably 2x low

internal/handlers/event_body.go:77-80, repeated in the commit message and the
PR body.

The doc comment states: "That is the bound: one body per concurrent download,
and a body is capped at 1 MB when it is ingested, so a download cannot cost
more than that."

Measured Go-heap allocation per download — production handler, discarding
ResponseWriter so no recorder buffer is counted, steady state after warm-up,
mean of 10 runs:

body=65536     alloc/download=175850    2.68x body
body=262144    alloc/download=574384    2.19x body
body=524288    alloc/download=1102961   2.10x body
body=1048576   alloc/download=2156499   2.06x body

alloc ~= 2 x body + ~45 KB constant, linear in body size. That is two
body-sized Go allocations per download, not one: the driver's column buffer,
plus the copy database/sql makes in convertAssign when scanning a []byte
column into a *[]byte. Both are live simultaneously during the copy, so peak
is ~2 bodies — ~2 MB at the ingest cap, not 1 MB. SQLite's own materialisation
of the column value lives in modernc's allocator outside the Go heap and is
therefore not included in the numbers above, so real process peak is at least
2x, not at most 1x.

Why it matters rather than being pedantry: this is the same class as the
finding it replaces — a memory bound asserted in a doc comment and in a commit
message that squash-merges to next permanently, which the code does not have.
An operator sizing for N concurrent downloads off "cannot cost more than that"
provisions half of what is needed.

What acceptable looks like: state ~2 bodies and name the second copy (the
database/sql scan clone), in the doc comment, the commit message and the PR
body — the three places the previous round required to agree.

This is a wording defect, not a design defect, and worth saying plainly: the
single read is correct and is genuinely single — one Raw(...).Row().Scan, no
second query, no preload, no association load, no AfterFind — and there is no
cheaper path through database/sql. Only the number is wrong. The previous
review had already computed ~2 MiB for a 1 MiB body, so if you judge a rework
cycle not worth a three-place text correction, that is a reasonable call to
overrule me on.

Verified

Findings 2 and 3 closed; both constraints intact after the rewrite; correctness,
authorization, mergeability, hygiene.

  • Mutation, route pattern body -> bodyy: TestSourceLogs_TruncationLinkDownloadsTheBody
    FAILs, 404 where 200 expected, "the link the page emits must be a live route".
  • Mutation, template href /source/ -> /sources/: FAILs
    TestSourceLogs_TruncationLinkDownloadsTheBody ("should have 2 item(s), but
    has 0") and TestHandleSourceLogs_TruncationMarkerLinksToDownload. Both
    reverted.
  • Finding 3: errShortBodyRead and the empty-chunk branch are gone.
    TestHandleEventBodyDownload_ReapedEvent404s covers both removal paths the
    codebase performs — soft delete and the reaper's Unscoped() hard delete —
    and both 404 with no Content-Length and no Content-Disposition.
  • Size class the suite missed, run over a real TCP socket through the
    production router (every existing test uses httptest.ResponseRecorder,
    which never reconciles Content-Length against the bytes framed on the
    wire): at the ingest cap and either side of it, 1048575 / 1048576 / 1048577
    bytes — sha256-identical, Content-Length exact, bytes on the wire exact,
    identity-framed not chunked, application/octet-stream +
    attachment + nosniff all present.
  • Slow client: httpWriteTimeout is 65s (internal/server/http.go:20), so the
    single Write cannot block indefinitely; dropping the flush loop changes the
    resident cost during a stall (whole body rather than one chunk) but not the
    failure mode.
  • Truncation link renders only under {{if .BodyTruncated}}, and
    BodyTruncated is BodyBytes > len(body), so it appears only when the body
    was genuinely cut.
  • make check exit 0 with private GOCACHE and GOLANGCI_LINT_CACHE: all 11
    packages ran with real durations, zero (cached), 0 issues.,
    fmt-check clean.
  • docker build --no-cache-filter=lint --no-cache-filter=builder . exit 0:
    #16 [lint 8/8] RUN make lint DONE 62.8s 0 issues.;
    #24 [builder 9/11] RUN make test DONE 66.0s; real per-package durations,
    zero (cached), neither stage CACHED. Image removed, docker ps -a empty,
    no prune of any kind run.
  • CI green on 6f37d05 (check / check, Successful in 3m23s). Fast-forwards
    onto next at c3b6623. Exactly one commit, title ends (closes #157),
    base next, TODO.md untouched, no Claude/Anthropic references or
    attribution trailers, no non-inclusive terminology, no scope creep, naming
    and idiom consistent with the surrounding handlers.

Disclosures

  • The mutations and the two probes were run as throwaway files driven by a
    filtered go test in a scratch clone, because make test cannot select a
    subset. Both authoritative gates were run only through make check and the
    Docker build. All probe files deleted; both clones git status clean.
  • The definition of done in #157 says
    "streams from the row rather than buffering it". This change buffers whole.
    I am treating that as a knowingly accepted deviation, not a finding: the PR
    documents why no streaming path exists through database/sql, and the same
    DoD sentence sanctions the 1 MB ingest cap as the bound.
FAIL — needs-rework Reviewed at `6f37d05` in an independent clone. Findings 2 and 3 of https://git.eeqj.de/sneak/webhooker/pulls/167#issuecomment-62493 are closed and I re-ran both mutations to prove it. Finding 1 is closed in substance — the chunking is gone and no chunk/streaming/peak-is-one-chunk claim survives in the code, the doc comments, the commit message or the PR body — but the bound that replaced it is still overstated, by 2x instead of 16x. ## Finding — "peak is one body per concurrent download" is measurably 2x low `internal/handlers/event_body.go:77-80`, repeated in the commit message and the PR body. The doc comment states: "That is the bound: one body per concurrent download, and a body is capped at 1 MB when it is ingested, so a download cannot cost more than that." Measured Go-heap allocation per download — production handler, discarding ResponseWriter so no recorder buffer is counted, steady state after warm-up, mean of 10 runs: ``` body=65536 alloc/download=175850 2.68x body body=262144 alloc/download=574384 2.19x body body=524288 alloc/download=1102961 2.10x body body=1048576 alloc/download=2156499 2.06x body ``` alloc ~= 2 x body + ~45 KB constant, linear in body size. That is two body-sized Go allocations per download, not one: the driver's column buffer, plus the copy `database/sql` makes in `convertAssign` when scanning a `[]byte` column into a `*[]byte`. Both are live simultaneously during the copy, so peak is ~2 bodies — ~2 MB at the ingest cap, not 1 MB. SQLite's own materialisation of the column value lives in modernc's allocator outside the Go heap and is therefore *not* included in the numbers above, so real process peak is at least 2x, not at most 1x. Why it matters rather than being pedantry: this is the same class as the finding it replaces — a memory bound asserted in a doc comment and in a commit message that squash-merges to `next` permanently, which the code does not have. An operator sizing for N concurrent downloads off "cannot cost more than that" provisions half of what is needed. What acceptable looks like: state ~2 bodies and name the second copy (the `database/sql` scan clone), in the doc comment, the commit message and the PR body — the three places the previous round required to agree. This is a wording defect, not a design defect, and worth saying plainly: the single read is correct and is genuinely single — one `Raw(...).Row().Scan`, no second query, no preload, no association load, no `AfterFind` — and there is no cheaper path through `database/sql`. Only the number is wrong. The previous review had already computed ~2 MiB for a 1 MiB body, so if you judge a rework cycle not worth a three-place text correction, that is a reasonable call to overrule me on. ## Verified Findings 2 and 3 closed; both constraints intact after the rewrite; correctness, authorization, mergeability, hygiene. - Mutation, route pattern `body` -> `bodyy`: `TestSourceLogs_TruncationLinkDownloadsTheBody` FAILs, 404 where 200 expected, "the link the page emits must be a live route". - Mutation, template `href` `/source/` -> `/sources/`: FAILs `TestSourceLogs_TruncationLinkDownloadsTheBody` ("should have 2 item(s), but has 0") and `TestHandleSourceLogs_TruncationMarkerLinksToDownload`. Both reverted. - Finding 3: `errShortBodyRead` and the empty-chunk branch are gone. `TestHandleEventBodyDownload_ReapedEvent404s` covers both removal paths the codebase performs — soft delete and the reaper's `Unscoped()` hard delete — and both 404 with no `Content-Length` and no `Content-Disposition`. - Size class the suite missed, run over a **real TCP socket** through the production router (every existing test uses `httptest.ResponseRecorder`, which never reconciles `Content-Length` against the bytes framed on the wire): at the ingest cap and either side of it, 1048575 / 1048576 / 1048577 bytes — sha256-identical, `Content-Length` exact, bytes on the wire exact, identity-framed not chunked, `application/octet-stream` + `attachment` + `nosniff` all present. - Slow client: `httpWriteTimeout` is 65s (`internal/server/http.go:20`), so the single `Write` cannot block indefinitely; dropping the flush loop changes the resident cost during a stall (whole body rather than one chunk) but not the failure mode. - Truncation link renders only under `{{if .BodyTruncated}}`, and `BodyTruncated` is `BodyBytes > len(body)`, so it appears only when the body was genuinely cut. - `make check` exit 0 with private `GOCACHE` and `GOLANGCI_LINT_CACHE`: all 11 packages ran with real durations, **zero** `(cached)`, `0 issues.`, fmt-check clean. - `docker build --no-cache-filter=lint --no-cache-filter=builder .` exit 0: `#16 [lint 8/8] RUN make lint` DONE 62.8s `0 issues.`; `#24 [builder 9/11] RUN make test` DONE 66.0s; real per-package durations, zero `(cached)`, neither stage CACHED. Image removed, `docker ps -a` empty, no prune of any kind run. - CI green on `6f37d05` (`check / check`, Successful in 3m23s). Fast-forwards onto `next` at `c3b6623`. Exactly one commit, title ends ` (closes #157)`, base `next`, `TODO.md` untouched, no Claude/Anthropic references or attribution trailers, no non-inclusive terminology, no scope creep, naming and idiom consistent with the surrounding handlers. ## Disclosures - The mutations and the two probes were run as throwaway files driven by a filtered `go test` in a scratch clone, because `make test` cannot select a subset. Both authoritative gates were run only through `make check` and the Docker build. All probe files deleted; both clones `git status` clean. - The definition of done in https://git.eeqj.de/sneak/webhooker/issues/157 says "streams from the row rather than buffering it". This change buffers whole. I am treating that as a knowingly accepted deviation, not a finding: the PR documents why no streaming path exists through `database/sql`, and the same DoD sentence sanctions the 1 MB ingest cap as the bound.
clawbot added needs-rework and removed needs-review labels 2026-08-18 00:12:02 +02:00
clawbot force-pushed issue-157-event-body-download from 6f37d05ab6 to 5a75f770a2 2026-08-18 00:19:30 +02:00 Compare
clawbot added needs-review and removed needs-rework labels 2026-08-18 00:27:36 +02:00
clawbot commented 2026-08-18 00:40:18 +02:00
Author
Collaborator
Copy Link

PASS

Scoped confirmation pass at 5a75f77 in an independent clone, round 3. I did not re-review the code: I relied on #167 (comment) and #167 (comment) for the single read, byte-identity and Content-Length across all size classes including over a real socket, both route mutations, the ownership mutation, the removal of errShortBodyRead, per-webhook file isolation as the real IDOR guard, the no-transaction trade, the headers, the non-steerable filename and the truncation-link condition. I verified only that those reviews still apply, and that the amended text is true.

Amend is text-only — verified two ways. git range-diff 6f37d05~1..6f37d05 5a75f77~1..5a75f77 shows exactly two hunks, commit message and the serveEventBody doc comment. Independently, diff of the two commits' full patches (git diff PARENT..COMMIT each side) is byte-identical apart from that comment block and its blob hash. No code, test, config or template byte moved.

The corrected claims are true — measured independently, not accepted. Production handler, ResponseWriter discarded, warm-up then mean of 10:

body=0        alloc=44791      -
body=65536    alloc=175968     2.685x
body=262144   alloc=574083     2.190x
body=524288   alloc=1103855    2.105x
body=1048576  alloc=2156663    2.057x
body=2097152  alloc=4263595    2.033x

Reproduces #issuecomment-62633 to within 0.1%. The ~45 KB constant is the body=0 row at 44791 bytes — right, and it is per-request HTTP/auth/ownedWebhook overhead, independent of body size.

Probing "two, not three": a scan-only probe with no HTTP layer gives 262144 -> 526066 (2.007x) and 1048576 -> 2098936 (2.002x). Exactly two body-sized Go allocations in the read path, no third, essentially zero constant. Source confirms which two and that they are simultaneously live: modernc.org/sqlite@v1.28.0/sqlite.go:873 v = make([]byte, len) copying out of SQLite's memory, then database/sql/convert.go:272 *d = bytes.Clone(s) allocating the second while the driver slice is still reachable through rows.lastcols. Liveness is established from source, not inferred from TotalAlloc.

convertAssign claim is correct for this exact scan target: src []byte (the cast(body as blob) is what makes it []byte) into dest *[]byte hits the bytes.Clone arm. Nothing on the write path adds a third — w.Write passes through bufio to the conn without a body-sized buffer.

"A floor, not a ceiling" is accurate. modernc.org/memory@v1.7.2 serves the driver's allocator from raw mmap syscalls (mmap_unix.go:34), so SQLite's materialised column value is invisible to runtime.MemStats and is genuinely additional RSS — the driver explicitly copies out of it into a fresh Go slice, so it is not the same buffer counted twice. Real peak is ~3 bodies.

The premise the bound rests on holds: ingest really is capped at 1 MB (internal/handlers/webhook.go:145-166, io.LimitReader plus a length check at 1 << 20), enforced in the receiver handler rather than by MaxBodySize middleware, which the /webhook/{uuid} route does not carry. "No cheaper bound" also holds: conn is unexported in the driver and there is no sqlite3_blob_open surface at all.

The PR body's "Accepted deviation from the definition of done" section states plainly that #157's DoD asks the route to stream from the row and that this change buffers whole. The pass is not silent about it.

Gate, run on 5a75f77 in this clone.

make check exit 0 with private GOCACHE and GOLANGCI_LINT_CACHE: 12 packages, real durations, zero (cached), 0 issues., fmt-check clean. make fmt leaves the tree clean.

docker build --no-cache-filter=lint --no-cache-filter=builder . exit 0:

#17 [lint 7/8] RUN make fmt-check      DONE 3.9s
#18 [lint 8/8] RUN make lint           DONE 53.6s   0 issues.
#26 [builder  9/11] RUN make test      DONE 56.6s

Neither stage CACHED, 12 packages with real per-package durations, zero (cached) anywhere in the build log. Image removed, docker ps -a clear of anything of mine, no prune of any kind run.

CI green on 5a75f77 (check / check, Successful in 3m1s). next moved to 5888d14 while this sat; git merge-tree against it is conflict-free, so it merges cleanly but is no longer a fast-forward — a non-issue for the repo's squash default. Exactly one commit, title ends (closes #157), base next, TODO.md untouched, no Claude/Anthropic references or attribution trailers.

Disclosures.

  • The allocation probes were a throwaway _test.go file driven by a filtered go test, because make test cannot select a subset. Both authoritative gates were run only through make check and the Docker build. Probe file deleted; git status clean.
  • The DoD deviation is documented, not hidden, and two prior reviewers accepted it — but #157 (comment) states "it must stream from the row rather than buffer" as a must-not-drop. Whether a documented deviation from that is acceptable is the owner's call, not a reviewer's; I am not failing on it.
PASS Scoped confirmation pass at `5a75f77` in an independent clone, round 3. I did not re-review the code: I relied on https://git.eeqj.de/sneak/webhooker/pulls/167#issuecomment-62493 and https://git.eeqj.de/sneak/webhooker/pulls/167#issuecomment-62633 for the single read, byte-identity and `Content-Length` across all size classes including over a real socket, both route mutations, the ownership mutation, the removal of `errShortBodyRead`, per-webhook file isolation as the real IDOR guard, the no-transaction trade, the headers, the non-steerable filename and the truncation-link condition. I verified only that those reviews still apply, and that the amended text is true. **Amend is text-only — verified two ways.** `git range-diff 6f37d05~1..6f37d05 5a75f77~1..5a75f77` shows exactly two hunks, commit message and the `serveEventBody` doc comment. Independently, `diff` of the two commits' full patches (`git diff PARENT..COMMIT` each side) is byte-identical apart from that comment block and its blob hash. No code, test, config or template byte moved. **The corrected claims are true — measured independently, not accepted.** Production handler, `ResponseWriter` discarded, warm-up then mean of 10: ``` body=0 alloc=44791 - body=65536 alloc=175968 2.685x body=262144 alloc=574083 2.190x body=524288 alloc=1103855 2.105x body=1048576 alloc=2156663 2.057x body=2097152 alloc=4263595 2.033x ``` Reproduces #issuecomment-62633 to within 0.1%. The `~45 KB` constant is the `body=0` row at 44791 bytes — right, and it is per-request HTTP/auth/`ownedWebhook` overhead, independent of body size. Probing "two, not three": a scan-only probe with no HTTP layer gives `262144 -> 526066 (2.007x)` and `1048576 -> 2098936 (2.002x)`. Exactly two body-sized Go allocations in the read path, no third, essentially zero constant. Source confirms which two and that they are simultaneously live: `modernc.org/sqlite@v1.28.0/sqlite.go:873` `v = make([]byte, len)` copying out of SQLite's memory, then `database/sql/convert.go:272` `*d = bytes.Clone(s)` allocating the second while the driver slice is still reachable through `rows.lastcols`. Liveness is established from source, not inferred from `TotalAlloc`. `convertAssign` claim is correct for this exact scan target: src `[]byte` (the `cast(body as blob)` is what makes it `[]byte`) into dest `*[]byte` hits the `bytes.Clone` arm. Nothing on the write path adds a third — `w.Write` passes through `bufio` to the conn without a body-sized buffer. "A floor, not a ceiling" is accurate. `modernc.org/memory@v1.7.2` serves the driver's allocator from raw `mmap` syscalls (`mmap_unix.go:34`), so SQLite's materialised column value is invisible to `runtime.MemStats` and is genuinely additional RSS — the driver explicitly copies *out of* it into a fresh Go slice, so it is not the same buffer counted twice. Real peak is ~3 bodies. The premise the bound rests on holds: ingest really is capped at 1 MB (`internal/handlers/webhook.go:145-166`, `io.LimitReader` plus a length check at `1 << 20`), enforced in the receiver handler rather than by `MaxBodySize` middleware, which the `/webhook/{uuid}` route does not carry. "No cheaper bound" also holds: `conn` is unexported in the driver and there is no `sqlite3_blob_open` surface at all. The PR body's "Accepted deviation from the definition of done" section states plainly that https://git.eeqj.de/sneak/webhooker/issues/157's DoD asks the route to stream from the row and that this change buffers whole. The pass is not silent about it. **Gate, run on `5a75f77` in this clone.** `make check` exit 0 with private `GOCACHE` and `GOLANGCI_LINT_CACHE`: 12 packages, real durations, **zero** `(cached)`, `0 issues.`, fmt-check clean. `make fmt` leaves the tree clean. `docker build --no-cache-filter=lint --no-cache-filter=builder .` exit 0: ``` #17 [lint 7/8] RUN make fmt-check DONE 3.9s #18 [lint 8/8] RUN make lint DONE 53.6s 0 issues. #26 [builder 9/11] RUN make test DONE 56.6s ``` Neither stage `CACHED`, 12 packages with real per-package durations, zero `(cached)` anywhere in the build log. Image removed, `docker ps -a` clear of anything of mine, no prune of any kind run. CI green on `5a75f77` (`check / check`, Successful in 3m1s). `next` moved to `5888d14` while this sat; `git merge-tree` against it is conflict-free, so it merges cleanly but is no longer a fast-forward — a non-issue for the repo's squash default. Exactly one commit, title ends ` (closes #157)`, base `next`, `TODO.md` untouched, no Claude/Anthropic references or attribution trailers. **Disclosures.** - The allocation probes were a throwaway `_test.go` file driven by a filtered `go test`, because `make test` cannot select a subset. Both authoritative gates were run only through `make check` and the Docker build. Probe file deleted; `git status` clean. - The DoD deviation is documented, not hidden, and two prior reviewers accepted it — but https://git.eeqj.de/sneak/webhooker/issues/157#issuecomment-62345 states "it must stream from the row rather than buffer" as a must-not-drop. Whether a documented deviation from that is acceptable is the owner's call, not a reviewer's; I am not failing on it.
clawbot merged commit 41ff16a817 into next 2026-08-18 00:41:32 +02:00
clawbot deleted branch issue-157-event-body-download 2026-08-18 00:41:32 +02:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label needs-review
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees clawbot
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#167
Delete Branch "issue-157-event-body-download"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?