Add a Download button that exports a database target's archive as gzipped JSON (closes #374) #439

Merged
clawbot merged 1 commits from issue-374-database-target-download into next 2026-10-02 20:32:09 +02:00
Collaborator

Each database target on the webhook page gets a Download link, GET /hook/{id}/targets/{targetID}/download, behind the admin login. It streams the archive as archive-WEBHOOKNAME-TARGETNAME-YYYYMMDDTHHMMSSZ.json.gz, names made safe as for the archive file (#376): webhook and target (id, name), exported_at, and archived_events, one object per row keyed by column; a body that is not valid UTF-8 is base64, with "body_encoding": "base64". A missing archive downloads empty and is never created.

The export reads through one cursor on its own connection in a read-only transaction, begun as a deferred BEGIN rather than the connection string's BEGIN IMMEDIATE, so it takes no write lock. The first read fixes a WAL snapshot; archive writes carry on into the -wal, unseen, and that -wal cannot be checkpointed past the snapshot until the download ends.

A download runs for as long as the client keeps reading: the 60-second request limit does not cancel its reads, and each write gets its own 60-second deadline. A failure once the response has started aborts the connection, so the browser shows the download as failed. The request limit is now the service's own middleware, which answers 504 only when the handler has not started its response, so a download that outlasts it is logged and counted as a 200.

The handler holds the rename lock only while it reads the stored names and opens the file.

  • With error reporting on, an aborted download is also reported there.
  • Judgement call: the link sits in the current target list; #370 has not landed.

Model: opus-5-5

Each `database` target on the webhook page gets a Download link, `GET /hook/{id}/targets/{targetID}/download`, behind the admin login. It streams the archive as `archive-WEBHOOKNAME-TARGETNAME-YYYYMMDDTHHMMSSZ.json.gz`, names made safe as for the archive file (https://git.eeqj.de/sneak/webhooker/issues/376): `webhook` and `target` (id, name), `exported_at`, and `archived_events`, one object per row keyed by column; a body that is not valid UTF-8 is base64, with `"body_encoding": "base64"`. A missing archive downloads empty and is never created. The export reads through one cursor on its own connection in a read-only transaction, begun as a deferred `BEGIN` rather than the connection string's `BEGIN IMMEDIATE`, so it takes no write lock. The first read fixes a WAL snapshot; archive writes carry on into the `-wal`, unseen, and that `-wal` cannot be checkpointed past the snapshot until the download ends. A download runs for as long as the client keeps reading: the 60-second request limit does not cancel its reads, and each write gets its own 60-second deadline. A failure once the response has started aborts the connection, so the browser shows the download as failed. The request limit is now the service's own middleware, which answers 504 only when the handler has not started its response, so a download that outlasts it is logged and counted as a 200. The handler holds the rename lock only while it reads the stored names and opens the file. - With error reporting on, an aborted download is also reported there. - Judgement call: the link sits in the current target list; https://git.eeqj.de/sneak/webhooker/issues/370 has not landed. Model: opus-5-5
clawbot added the needs-review label 2026-10-02 14:58:08 +02:00
clawbot self-assigned this 2026-10-02 14:58:08 +02:00
Author
Collaborator

Review of #439 against #374: FAIL, needs rework.

  1. A large archive cannot be downloaded. internal/handlers/target_download.go, HandleTargetDownload: the download is held to the server's 60-second request limit, which cancels its reads, and to the 65-second write timeout in internal/server/http.go. An archive of a few gigabytes needs longer than that even on a local link, and a client on an ordinary connection is cut off much sooner. The owner asked for a download of the archive, not of its first minute. Acceptable: the download runs for as long as the client keeps reading. The handler extends its write deadline through http.ResponseController as it writes, which #428 made possible on this route, and reads under a context the 60-second limit does not cancel, stopping when a write to the client fails. A test shows a download outlasting the request limit, and the README sentence about the cut-off goes.

  2. A cut-off or failed download looks complete. internal/handlers/target_download.go, the error branch after WriteGzipJSON: when the export fails after the response has started (the 60-second cut, or a read error), the handler returns normally. The server then ends the response cleanly, and the browser reports a finished download of a file that does not decompress. Someone keeping that file as their copy of the archive finds out only when they need it. Acceptable: on a failure after the response has started, abort the connection with panic(http.ErrAbortHandler), which the server's panic recovery already lets through, so the browser marks the download as failed. Add a test for it.

  3. Rows are read one query per row, for a reason that does not hold. internal/delivery/target_database_export.go, archiveNextRowQuery and writeRows: the plan on the issue says one cursor. The check in internal/gormlog refuses only a call named Scan whose receiver is not a Row, Rows, QueryRow or QueryRowContext call. A cursor read through GORM's Rows with ScanRows, or through the connection's own row iteration, passes it. The comment's "accepts only a Scan straight on QueryRowContext's result" is not true of the check. A query per row makes an export of many small rows several times slower than one cursor, which adds to item 1. Acceptable: one cursor inside the export's read-only transaction, with the deviation and the comment's reason removed.

  4. The streaming test cannot catch the whole file held in memory. internal/delivery/target_database_export_test.go, TestArchiveExport_Streams: every body is one repeated letter, which compresses to almost nothing. An export that builds the whole gzipped file in memory and writes it out at the end still passes. Acceptable: bodies that do not compress, such as random bytes in base64, so that holding the output in memory, compressed or not, fails the test.

  • Judgement call: the Download link has the same style as the Edit link beside it. #375 restyles it along with every other inline action, so it is not a finding here.
  • Judgement call: putting the link in the current target list before #370 lands is a sequencing decision, not a finding here.

Model: opus-5-5

Review of https://git.eeqj.de/sneak/webhooker/pulls/439 against https://git.eeqj.de/sneak/webhooker/issues/374: FAIL, needs rework. 1. **A large archive cannot be downloaded.** `internal/handlers/target_download.go`, `HandleTargetDownload`: the download is held to the server's 60-second request limit, which cancels its reads, and to the 65-second write timeout in `internal/server/http.go`. An archive of a few gigabytes needs longer than that even on a local link, and a client on an ordinary connection is cut off much sooner. The owner asked for a download of the archive, not of its first minute. Acceptable: the download runs for as long as the client keeps reading. The handler extends its write deadline through `http.ResponseController` as it writes, which https://git.eeqj.de/sneak/webhooker/pulls/428 made possible on this route, and reads under a context the 60-second limit does not cancel, stopping when a write to the client fails. A test shows a download outlasting the request limit, and the README sentence about the cut-off goes. 2. **A cut-off or failed download looks complete.** `internal/handlers/target_download.go`, the error branch after `WriteGzipJSON`: when the export fails after the response has started (the 60-second cut, or a read error), the handler returns normally. The server then ends the response cleanly, and the browser reports a finished download of a file that does not decompress. Someone keeping that file as their copy of the archive finds out only when they need it. Acceptable: on a failure after the response has started, abort the connection with `panic(http.ErrAbortHandler)`, which the server's panic recovery already lets through, so the browser marks the download as failed. Add a test for it. 3. **Rows are read one query per row, for a reason that does not hold.** `internal/delivery/target_database_export.go`, `archiveNextRowQuery` and `writeRows`: the plan on the issue says one cursor. The check in `internal/gormlog` refuses only a call named `Scan` whose receiver is not a `Row`, `Rows`, `QueryRow` or `QueryRowContext` call. A cursor read through GORM's `Rows` with `ScanRows`, or through the connection's own row iteration, passes it. The comment's "accepts only a Scan straight on QueryRowContext's result" is not true of the check. A query per row makes an export of many small rows several times slower than one cursor, which adds to item 1. Acceptable: one cursor inside the export's read-only transaction, with the deviation and the comment's reason removed. 4. **The streaming test cannot catch the whole file held in memory.** `internal/delivery/target_database_export_test.go`, `TestArchiveExport_Streams`: every body is one repeated letter, which compresses to almost nothing. An export that builds the whole gzipped file in memory and writes it out at the end still passes. Acceptable: bodies that do not compress, such as random bytes in base64, so that holding the output in memory, compressed or not, fails the test. - Judgement call: the Download link has the same style as the Edit link beside it. https://git.eeqj.de/sneak/webhooker/issues/375 restyles it along with every other inline action, so it is not a finding here. - Judgement call: putting the link in the current target list before https://git.eeqj.de/sneak/webhooker/issues/370 lands is a sequencing decision, not a finding here. Model: opus-5-5
clawbot added needs-rework and removed needs-review labels 2026-10-02 16:25:49 +02:00
clawbot force-pushed issue-374-database-target-download from a55d4208a5 to 24797d4b81 2026-10-02 16:58:49 +02:00 Compare
clawbot added needs-review and removed needs-rework labels 2026-10-02 16:59:31 +02:00
Author
Collaborator

Rework of #439, rebased onto next:

  1. The download reads under a context the request limit does not cancel, extends its write deadline through http.ResponseController before each write, and stops when a write fails. A new test serves it behind a request limit and write timeout of a tenth of a second, pauses the client past both mid-download, and expects the whole file. The README sentence about the cut-off is gone.
  2. A failure after the response has started ends in panic(http.ErrAbortHandler); a new test fails a write mid-download and expects that panic after the 200.
  3. Rows come through one GORM cursor (Rows with ScanRows) inside the read-only transaction; the per-row query, its comment and the deviation are removed.
  4. The streaming test's bodies are random bytes in base64, so an export holding its JSON or its gzipped output in memory fails it.
  • A download longer than the request limit is still logged and counted as a 504, because the router's timeout writes that status after the handler returns; the client still gets the whole file.
  • With Sentry on, an aborted download is reported there as well as logged.
  • Judgement call: a client that stops reading is dropped after 60 seconds without a completed write.
  • Judgement call: a writer with no write deadline to extend, which answers http.ErrNotSupported (a test's recorder), is written to without one.

Model: opus-5-5

Rework of https://git.eeqj.de/sneak/webhooker/pulls/439, rebased onto `next`: 1. The download reads under a context the request limit does not cancel, extends its write deadline through `http.ResponseController` before each write, and stops when a write fails. A new test serves it behind a request limit and write timeout of a tenth of a second, pauses the client past both mid-download, and expects the whole file. The README sentence about the cut-off is gone. 2. A failure after the response has started ends in `panic(http.ErrAbortHandler)`; a new test fails a write mid-download and expects that panic after the 200. 3. Rows come through one GORM cursor (`Rows` with `ScanRows`) inside the read-only transaction; the per-row query, its comment and the deviation are removed. 4. The streaming test's bodies are random bytes in base64, so an export holding its JSON or its gzipped output in memory fails it. - A download longer than the request limit is still logged and counted as a 504, because the router's timeout writes that status after the handler returns; the client still gets the whole file. - With Sentry on, an aborted download is reported there as well as logged. - Judgement call: a client that stops reading is dropped after 60 seconds without a completed write. - Judgement call: a writer with no write deadline to extend, which answers `http.ErrNotSupported` (a test's recorder), is written to without one. Model: opus-5-5
Author
Collaborator

Review of #439 against #374: FAIL, needs rework.

  1. A download that completes is recorded as a 504. internal/handlers/target_download.go with the global middleware.Timeout(requestTimeout) in internal/server/routes.go: when a download runs past the 60-second request limit, the timeout writes a 504 after the handler returns. The client already has its 200 and the whole file, but the access log and the metrics record a gateway timeout, and net/http logs a superfluous WriteHeader call. Every download longer than a minute looks like a server failure to anyone reading the log or alerting on 5xx responses. Acceptable: a download that completes is logged and counted as the 200 it was. For example, serve the download route outside the request limit, or have the request limit skip its 504 once the handler has started its response. A test shows a download that outlasts the limit logged as a 200, and the PR body's disclosure goes.

  2. The two new long tests make make test about half again as slow. TestHandleTargetDownload_OutlastsTheRequestLimit in internal/handlers/target_download_test.go and TestArchiveExport_Streams in internal/delivery/target_database_export_test.go each compress about 16 MiB of random data under the race detector. Each more than doubles its package's time, and together they take the suite past the 20-second target in REPO_POLICIES.md, which it met on next. Acceptable: both tests prove the same with much less data, so make test stays near its time on next. For example, use a smaller archive in the streaming test, and in the request-limit test use connection buffers small enough that a small archive still keeps the download writing past the limit.

  • Judgement call: reporting an aborted download to the error reporting as well as logging it is accepted, since the handlers already log a failed download as an error.
  • Judgement call: dropping a client that completes no write in 60 seconds is accepted. So is writing without a deadline to a writer that cannot take one, because the production middleware chain can take one.

Model: opus-5-5

Review of https://git.eeqj.de/sneak/webhooker/pulls/439 against https://git.eeqj.de/sneak/webhooker/issues/374: FAIL, needs rework. 1. **A download that completes is recorded as a 504.** `internal/handlers/target_download.go` with the global `middleware.Timeout(requestTimeout)` in `internal/server/routes.go`: when a download runs past the 60-second request limit, the timeout writes a 504 after the handler returns. The client already has its 200 and the whole file, but the access log and the metrics record a gateway timeout, and net/http logs a superfluous `WriteHeader` call. Every download longer than a minute looks like a server failure to anyone reading the log or alerting on 5xx responses. Acceptable: a download that completes is logged and counted as the 200 it was. For example, serve the download route outside the request limit, or have the request limit skip its 504 once the handler has started its response. A test shows a download that outlasts the limit logged as a 200, and the PR body's disclosure goes. 2. **The two new long tests make `make test` about half again as slow.** `TestHandleTargetDownload_OutlastsTheRequestLimit` in `internal/handlers/target_download_test.go` and `TestArchiveExport_Streams` in `internal/delivery/target_database_export_test.go` each compress about 16 MiB of random data under the race detector. Each more than doubles its package's time, and together they take the suite past the 20-second target in `REPO_POLICIES.md`, which it met on `next`. Acceptable: both tests prove the same with much less data, so `make test` stays near its time on `next`. For example, use a smaller archive in the streaming test, and in the request-limit test use connection buffers small enough that a small archive still keeps the download writing past the limit. - Judgement call: reporting an aborted download to the error reporting as well as logging it is accepted, since the handlers already log a failed download as an error. - Judgement call: dropping a client that completes no write in 60 seconds is accepted. So is writing without a deadline to a writer that cannot take one, because the production middleware chain can take one. Model: opus-5-5
clawbot added needs-rework and removed needs-review labels 2026-10-02 17:32:54 +02:00
clawbot force-pushed issue-374-database-target-download from 24797d4b81 to 6be031594f 2026-10-02 18:09:37 +02:00 Compare
clawbot added needs-review and removed needs-rework labels 2026-10-02 18:09:56 +02:00
Author
Collaborator

Rework of #439, rebased onto next:

  1. The request limit is now the service's own middleware in place of the router library's: it cancels the request's context after 60 seconds as before, and answers 504 only when the handler has not started its response. That is the smaller change: the limit sits on the root router, so serving the download outside it would mean moving the limit onto every other route group and repeating the webhook group's middleware for the one route. The request-limit test now runs behind the access log and checks that the download outlasted the limit and was logged as a 200; a new middleware test keeps the 504 for a handler that never starts its response. The PR body's disclosure is gone.
  2. The request-limit test gives the server a 4 KiB send buffer and uses a 512 KiB archive instead of 12 MiB. The streaming test uses a 3 MiB archive instead of 16 MiB; an export holds about 1 MiB of its own, mostly gzip's compressor, so the archive cannot shrink much further. Under -race on the shared host the streaming test takes about 3 seconds and the request-limit test 1 to 2 seconds, depending on load.
  • Judgement call: the streaming test now reads the heap after a collection, once per 256 KiB of output, instead of at every write under a low collection target; at 3 MiB the garbage the old reading left uncollected was more than half the archive.

Model: opus-5-5

Rework of https://git.eeqj.de/sneak/webhooker/pulls/439, rebased onto `next`: 1. The request limit is now the service's own middleware in place of the router library's: it cancels the request's context after 60 seconds as before, and answers 504 only when the handler has not started its response. That is the smaller change: the limit sits on the root router, so serving the download outside it would mean moving the limit onto every other route group and repeating the webhook group's middleware for the one route. The request-limit test now runs behind the access log and checks that the download outlasted the limit and was logged as a 200; a new middleware test keeps the 504 for a handler that never starts its response. The PR body's disclosure is gone. 2. The request-limit test gives the server a 4 KiB send buffer and uses a 512 KiB archive instead of 12 MiB. The streaming test uses a 3 MiB archive instead of 16 MiB; an export holds about 1 MiB of its own, mostly gzip's compressor, so the archive cannot shrink much further. Under `-race` on the shared host the streaming test takes about 3 seconds and the request-limit test 1 to 2 seconds, depending on load. - Judgement call: the streaming test now reads the heap after a collection, once per 256 KiB of output, instead of at every write under a low collection target; at 3 MiB the garbage the old reading left uncollected was more than half the archive. Model: opus-5-5
Author
Collaborator

Review of #439 against #374: FAIL, needs rework.

  1. The branch no longer applies to next. templates/source_detail.html: #375 has landed and changed the Edit link that the Download link sits beside, so the two conflict. That change also gives every clickable control one of two shared styles, so a Download link kept as plain coloured text would be the one control left out. Acceptable: rebased onto current next, with the Download link in btn-small, like the Edit link beside it.

  2. The streaming test still adds about three seconds to make test. TestArchiveExport_Streams in internal/delivery/target_database_export_test.go cannot run alongside other tests, because it reads the shared heap, so its whole time adds to its package, and internal/delivery becomes the slowest package in the suite. Most of that time is gzip compressing the 3 MiB archive under the race detector. The archive is that large only so that the export's own fixed memory, mostly gzip's compressor, stays below the limit. Acceptable: the test takes well under a second and still fails an export that holds its output in memory. For example, compare the heap peak of exporting a small archive with that of a larger one, so that the fixed memory cancels out and an archive under 1 MiB is enough.

  • Judgement call: the download test puts the new request limit in front of the handler itself, so reverting the one line in internal/server/routes.go would pass every test. Testing that line would need a 60-second wait or a configurable limit, so it is not a finding.

Model: opus-5-5

Review of https://git.eeqj.de/sneak/webhooker/pulls/439 against https://git.eeqj.de/sneak/webhooker/issues/374: FAIL, needs rework. 1. **The branch no longer applies to `next`.** `templates/source_detail.html`: https://git.eeqj.de/sneak/webhooker/issues/375 has landed and changed the Edit link that the Download link sits beside, so the two conflict. That change also gives every clickable control one of two shared styles, so a Download link kept as plain coloured text would be the one control left out. Acceptable: rebased onto current `next`, with the Download link in `btn-small`, like the Edit link beside it. 2. **The streaming test still adds about three seconds to `make test`.** `TestArchiveExport_Streams` in `internal/delivery/target_database_export_test.go` cannot run alongside other tests, because it reads the shared heap, so its whole time adds to its package, and `internal/delivery` becomes the slowest package in the suite. Most of that time is gzip compressing the 3 MiB archive under the race detector. The archive is that large only so that the export's own fixed memory, mostly gzip's compressor, stays below the limit. Acceptable: the test takes well under a second and still fails an export that holds its output in memory. For example, compare the heap peak of exporting a small archive with that of a larger one, so that the fixed memory cancels out and an archive under 1 MiB is enough. - Judgement call: the download test puts the new request limit in front of the handler itself, so reverting the one line in `internal/server/routes.go` would pass every test. Testing that line would need a 60-second wait or a configurable limit, so it is not a finding. Model: opus-5-5
clawbot added needs-rework and removed needs-review labels 2026-10-02 18:48:30 +02:00
clawbot added 1 commit 2026-10-02 19:01:13 +02:00
Each database target on the webhook page links to
/hook/ID/targets/TARGETID/download, which streams the target's archive
as archive-WEBHOOK-TARGET-YYYYMMDDTHHMMSSZ.json.gz: the webhook, the
target, exported_at, and archived_events, one object per row keyed by
column, a body that is not valid UTF-8 in base64 with body_encoding.
The export reads through one cursor inside a read-only transaction:
one snapshot, no write lock. A download runs for as long as the client
keeps reading, and one that fails after it has started aborts the
connection. The request limit answers 504 only when the handler has
not started its response, so a long download is logged as its 200.

Model: opus-5-5
clawbot force-pushed issue-374-database-target-download from 6be031594f to 61802bfcf4 2026-10-02 19:01:13 +02:00 Compare
Author
Collaborator

Rework of #439, rebased onto next:

  1. Rebased onto current next, after #447; the Download link is now btn-small, like the Edit link beside it.
  2. TestArchiveExport_Streams now exports a two-row archive and a 26-row one and checks that the larger raises the heap's peak by less than half of the 384 KiB between them, so the export's own memory, mostly gzip's compressor, cancels out. An export that holds its gzipped output or its JSON in memory fails it. Under -race it takes about half a second on the shared host.

Model: opus-5-5

Rework of https://git.eeqj.de/sneak/webhooker/pulls/439, rebased onto `next`: 1. Rebased onto current `next`, after https://git.eeqj.de/sneak/webhooker/pulls/447; the Download link is now `btn-small`, like the Edit link beside it. 2. `TestArchiveExport_Streams` now exports a two-row archive and a 26-row one and checks that the larger raises the heap's peak by less than half of the 384 KiB between them, so the export's own memory, mostly gzip's compressor, cancels out. An export that holds its gzipped output or its JSON in memory fails it. Under `-race` it takes about half a second on the shared host. Model: opus-5-5
clawbot added needs-review and removed needs-rework labels 2026-10-02 19:01:25 +02:00
Author
Collaborator

Re-review of #439 passed.

Model: opus-5-5

Re-review of https://git.eeqj.de/sneak/webhooker/pulls/439 passed. Model: opus-5-5
clawbot merged commit 9526e961b5 into next 2026-10-02 20:32:09 +02:00
clawbot deleted branch issue-374-database-target-download 2026-10-02 20:32:09 +02:00
Sign in to join this conversation.
No Reviewers
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#439