Rotate a database target's archive monthly, daily or hourly (closes #379) #482

Merged
clawbot merged 1 commits from issue-379-archive-rotation into next 2026-10-03 04:18:38 +02:00
Collaborator

A database target now has a rotation setting beside its expiry (none, monthly, daily or hourly), on the new webhook page and both target forms and shown in the target list. A rotating target writes each event to a file named for the UTC period of its receive time (…-2026-10.db, …-2026-10-01.db, …-2026-10-01-19.db), as planned on #379.

  • A rename moves every file, keeping its period; it is refused if a new name is taken, and moves the files back if one fails.
  • The sweep prunes every file, one at a time under the target's lock, and deletes a rotated file it leaves empty.
  • Download lists the files, then opens one at a time, the file without a period first, then oldest period first, finding each again under the target's current names so a rename meanwhile loses none. Rows from a rotated file carry period.
  • The target list names the file an event received now goes to, and totals size and last write over all files.
  • A changed rotation applies from the next event; earlier files stay.

Disclosures:

  • Judgement call: Download writes each file as it stood when opened, not the archive as it stood at the start.
  • Judgement call: files are ordered by the period in their names, which after a mid-period change of granularity can differ from write order.
  • Deviation: a rename no longer moves a lone -wal or -shm whose .db is missing.
  • Judgement call: the test that Download keeps one file open reads /proc/self/fd, so it is skipped off Linux.

Model: opus-5-5

A database target now has a rotation setting beside its expiry (none, monthly, daily or hourly), on the new webhook page and both target forms and shown in the target list. A rotating target writes each event to a file named for the UTC period of its receive time (`…-2026-10.db`, `…-2026-10-01.db`, `…-2026-10-01-19.db`), as planned on https://git.eeqj.de/sneak/webhooker/issues/379. - A rename moves every file, keeping its period; it is refused if a new name is taken, and moves the files back if one fails. - The sweep prunes every file, one at a time under the target's lock, and deletes a rotated file it leaves empty. - Download lists the files, then opens one at a time, the file without a period first, then oldest period first, finding each again under the target's current names so a rename meanwhile loses none. Rows from a rotated file carry `period`. - The target list names the file an event received now goes to, and totals size and last write over all files. - A changed rotation applies from the next event; earlier files stay. Disclosures: - Judgement call: Download writes each file as it stood when opened, not the archive as it stood at the start. - Judgement call: files are ordered by the period in their names, which after a mid-period change of granularity can differ from write order. - Deviation: a rename no longer moves a lone `-wal` or `-shm` whose `.db` is missing. - Judgement call: the test that Download keeps one file open reads `/proc/self/fd`, so it is skipped off Linux. Model: opus-5-5
clawbot added the needs-review label 2026-10-03 01:55:03 +02:00
clawbot self-assigned this 2026-10-03 01:55:03 +02:00
Author
Collaborator

Review of #482 against #379: needs rework.

  1. Download opens every file at once. OpenArchiveExport in internal/delivery/target_database_export.go opens all of a target's files, each with its own connection and read transaction, before writing any of them out, and openTargetArchive in internal/handlers/target_download.go holds renameMu, the lock that webhook edits, target edits and target creation hold, while it does. An hourly target reaches thousands of files (720 with a 30-day expiry, 8760 with 365 days, no limit with never), so one download holds thousands of open files and connections, its memory grows with the number of files instead of staying bounded, and every edit and target creation waits until all of them are open. Acceptable: at most one archive file open at a time. List the files under renameMu, then open each one only when its rows are about to be written out, and close it before opening the next. A rename during the download must still not lose a file (for example, find each file again by its period under the target's current name, holding renameMu just long enough to do so). A file that is gone by the time it is reached (emptied by the sweep, or moved away) is skipped, as a missing archive is today. Update the doc comments, the README's Download paragraphs and the PR body to match, and add a test showing that a download of a target with many files never has more than one of them open.

  2. The sweep holds a target's lock across all of its files. sweepExpired in internal/delivery/target_database_archive.go takes the per-target lock the write path uses once, then opens, migrates, prunes and counts every file before letting go. For an hourly target with thousands of files, every write to that target waits for the whole sweep, at every sweep interval. Deliveries waiting on it can occupy all ten delivery workers, which stalls deliveries to every webhook. Acceptable: take the lock for one file at a time, so a write waits for at most one file's prune, and skip a file that is gone when it is reached. Update the sweepExpired comment and the README's sweep paragraph to match.

  3. Moving files back after a failed rename is not tested across files. The only move-back test, TestArchiveWriter_RenameMovesBackOnFailure in internal/delivery/target_database_test.go, renames a target that has a single file. Nothing checks that files already moved are moved back when a later file of the same target fails to move. Acceptable: a test that renames a target with a file without a period and at least one file named for a period, where a later file fails to move (for example, a new name that fits the file without a period but is too long once a period is added). It must show that every file is back under its old name with its rows, and that nothing is left under the new name.

  4. Red on current next. Rebased onto current next, make check fails in TestPausedTarget_ShownUntilBreakerCloses (internal/handlers/target_paused_test.go). That test, from #385, checks that no "UTC" appears in the webhook page's targets section, and the new rotation help text in the add target form ("… or hour (UTC) …") is inside that section. Acceptable: rebase onto next and make make check green with that check still meaningful, for example by checking only the paused target's own row instead of the whole section, not by dropping the check.

Judgement call: the disclosed deviation is accepted. A rename leaves a lone -wal or -shm whose .db is missing under the old name; it cannot be read as part of the archive, and moving it would put it beside the next file created under the new name.

Judgement call: the PR body is a little over 250 words and is not raised here; keep it within that after the rework.

Model: opus-5-5

Review of https://git.eeqj.de/sneak/webhooker/pulls/482 against https://git.eeqj.de/sneak/webhooker/issues/379: needs rework. 1. **Download opens every file at once.** `OpenArchiveExport` in `internal/delivery/target_database_export.go` opens all of a target's files, each with its own connection and read transaction, before writing any of them out, and `openTargetArchive` in `internal/handlers/target_download.go` holds `renameMu`, the lock that webhook edits, target edits and target creation hold, while it does. An hourly target reaches thousands of files (720 with a 30-day expiry, 8760 with 365 days, no limit with `never`), so one download holds thousands of open files and connections, its memory grows with the number of files instead of staying bounded, and every edit and target creation waits until all of them are open. Acceptable: at most one archive file open at a time. List the files under `renameMu`, then open each one only when its rows are about to be written out, and close it before opening the next. A rename during the download must still not lose a file (for example, find each file again by its period under the target's current name, holding `renameMu` just long enough to do so). A file that is gone by the time it is reached (emptied by the sweep, or moved away) is skipped, as a missing archive is today. Update the doc comments, the README's Download paragraphs and the PR body to match, and add a test showing that a download of a target with many files never has more than one of them open. 2. **The sweep holds a target's lock across all of its files.** `sweepExpired` in `internal/delivery/target_database_archive.go` takes the per-target lock the write path uses once, then opens, migrates, prunes and counts every file before letting go. For an hourly target with thousands of files, every write to that target waits for the whole sweep, at every sweep interval. Deliveries waiting on it can occupy all ten delivery workers, which stalls deliveries to every webhook. Acceptable: take the lock for one file at a time, so a write waits for at most one file's prune, and skip a file that is gone when it is reached. Update the `sweepExpired` comment and the README's sweep paragraph to match. 3. **Moving files back after a failed rename is not tested across files.** The only move-back test, `TestArchiveWriter_RenameMovesBackOnFailure` in `internal/delivery/target_database_test.go`, renames a target that has a single file. Nothing checks that files already moved are moved back when a later file of the same target fails to move. Acceptable: a test that renames a target with a file without a period and at least one file named for a period, where a later file fails to move (for example, a new name that fits the file without a period but is too long once a period is added). It must show that every file is back under its old name with its rows, and that nothing is left under the new name. 4. **Red on current `next`.** Rebased onto current `next`, `make check` fails in `TestPausedTarget_ShownUntilBreakerCloses` (`internal/handlers/target_paused_test.go`). That test, from https://git.eeqj.de/sneak/webhooker/issues/385, checks that no "UTC" appears in the webhook page's targets section, and the new rotation help text in the add target form ("… or hour (UTC) …") is inside that section. Acceptable: rebase onto `next` and make `make check` green with that check still meaningful, for example by checking only the paused target's own row instead of the whole section, not by dropping the check. Judgement call: the disclosed deviation is accepted. A rename leaves a lone `-wal` or `-shm` whose `.db` is missing under the old name; it cannot be read as part of the archive, and moving it would put it beside the next file created under the new name. Judgement call: the PR body is a little over 250 words and is not raised here; keep it within that after the rework. Model: opus-5-5
clawbot added needs-rework and removed needs-review labels 2026-10-03 02:23:58 +02:00
clawbot force-pushed issue-379-archive-rotation from 13f881007a to e55134532c 2026-10-03 02:51:57 +02:00 Compare
clawbot added needs-review and removed needs-rework labels 2026-10-03 02:52:42 +02:00
Author
Collaborator

Rework for the review above:

  1. Download lists the files under renameMu, then opens one at a time as its rows are written out and closes it before the next, taking renameMu only to find each file by its period under the names stored then and open it; a file that is gone is skipped. Doc comments, the README's Download paragraphs and the PR body match. New tests: TestArchiveExport_OneFileOpenAtATime (24 hourly files, never more than one open) and TestHandleTargetDownload_FindsFilesAfterRename (target renamed mid-download, no file lost).
  2. The sweep lists the files under the target's lock, then takes it again for one file at a time and skips a file that is gone; the sweepExpired comment and the README's sweep paragraph match.
  3. TestArchiveWriter_RenameMovesBackEveryFile: a file without a period and a month's file, with a 251-byte new name the month's file cannot take; both files end up under the old name with their rows, and nothing is under the new name.
  4. Rebased onto next; the "UTC" check now looks only at the paused target's own row.

Disclosures:

  • Judgement call: a download looks up the stored names including deleted rows, so a target or webhook deleted during its download still downloads in full.
  • Judgement call: each file is written out as it stood when it was opened, not the whole archive as it stood at the start.
  • Judgement call: the one-file-open test reads /proc/self/fd, so it is skipped off Linux.

Model: opus-5-5

Rework for the review above: 1. Download lists the files under `renameMu`, then opens one at a time as its rows are written out and closes it before the next, taking `renameMu` only to find each file by its period under the names stored then and open it; a file that is gone is skipped. Doc comments, the README's Download paragraphs and the PR body match. New tests: `TestArchiveExport_OneFileOpenAtATime` (24 hourly files, never more than one open) and `TestHandleTargetDownload_FindsFilesAfterRename` (target renamed mid-download, no file lost). 2. The sweep lists the files under the target's lock, then takes it again for one file at a time and skips a file that is gone; the `sweepExpired` comment and the README's sweep paragraph match. 3. `TestArchiveWriter_RenameMovesBackEveryFile`: a file without a period and a month's file, with a 251-byte new name the month's file cannot take; both files end up under the old name with their rows, and nothing is under the new name. 4. Rebased onto `next`; the "UTC" check now looks only at the paused target's own row. Disclosures: - Judgement call: a download looks up the stored names including deleted rows, so a target or webhook deleted during its download still downloads in full. - Judgement call: each file is written out as it stood when it was opened, not the whole archive as it stood at the start. - Judgement call: the one-file-open test reads `/proc/self/fd`, so it is skipped off Linux. Model: opus-5-5
Author
Collaborator

Re-review of #482 against #379: needs rework.

  1. Conflicts with current next. Rebased onto current next, static/css/tailwind.css conflicts. Acceptable: rebase onto next and regenerate that file with make css rather than merging it by hand.

  2. The one-file-open test makes the suite much slower. openFilesPeak in internal/delivery/target_database_export_test.go, which TestArchiveExport_OneFileOpenAtATime writes the export to, lists /proc/self/fd and resolves every entry at each of gzip's small writes. That roughly triples the run time of the internal/delivery package compared with next, making it the slowest package and narrowing the margin to the per-package time limit in script/test under the host loads that script describes. Acceptable: check the open files once per few KiB of output, for example by writing through a bufio writer as TestArchiveExport_Streams in the same file already does, so the package runs about as long as on next and the test still fails when every file is opened at once.

  3. Commit message too long. The commit message body is about 130 words, over the limit of about 120. Acceptable: 120 words or fewer, keeping the subject line and the Model: line.

Judgement call: the three judgement calls disclosed in the rework comment are accepted.

Judgement call: a file that disappears between Download's check that it exists and its open still ends the download instead of being skipped; next treats a missing archive the same way, which the earlier finding asked to match, so it is not raised.

Model: opus-5-5

Re-review of https://git.eeqj.de/sneak/webhooker/pulls/482 against https://git.eeqj.de/sneak/webhooker/issues/379: needs rework. 1. **Conflicts with current `next`.** Rebased onto current `next`, `static/css/tailwind.css` conflicts. Acceptable: rebase onto `next` and regenerate that file with `make css` rather than merging it by hand. 2. **The one-file-open test makes the suite much slower.** `openFilesPeak` in `internal/delivery/target_database_export_test.go`, which `TestArchiveExport_OneFileOpenAtATime` writes the export to, lists `/proc/self/fd` and resolves every entry at each of gzip's small writes. That roughly triples the run time of the `internal/delivery` package compared with `next`, making it the slowest package and narrowing the margin to the per-package time limit in `script/test` under the host loads that script describes. Acceptable: check the open files once per few KiB of output, for example by writing through a `bufio` writer as `TestArchiveExport_Streams` in the same file already does, so the package runs about as long as on `next` and the test still fails when every file is opened at once. 3. **Commit message too long.** The commit message body is about 130 words, over the limit of about 120. Acceptable: 120 words or fewer, keeping the subject line and the `Model:` line. Judgement call: the three judgement calls disclosed in the rework comment are accepted. Judgement call: a file that disappears between Download's check that it exists and its open still ends the download instead of being skipped; `next` treats a missing archive the same way, which the earlier finding asked to match, so it is not raised. Model: opus-5-5
clawbot added needs-rework and removed needs-review labels 2026-10-03 03:59:08 +02:00
clawbot added 1 commit 2026-10-03 04:06:25 +02:00
A database target's rotation (none, monthly, daily or hourly) puts the
UTC period of each event's receive time in its archive file name, so
each file holds exactly its period's events. It is on the new webhook
page, the add target form and the target edit form, and shown in the
target list.

Renames move every one of a target's files and move them back if one
fails. The sweep prunes every file, one at a time under the target's
lock, and deletes a rotated file it leaves empty. Download lists the
files, then opens one at a time, oldest first, finding each again under
the target's current names, and gives each row its period. The target
list names the current file and totals the size of all of them.

Model: opus-5-5
clawbot force-pushed issue-379-archive-rotation from e55134532c to ed344ecc66 2026-10-03 04:06:25 +02:00 Compare
Author
Collaborator

Rework for the re-review above:

  1. Rebased onto next, with static/css/tailwind.css regenerated by make css.
  2. TestArchiveExport_OneFileOpenAtATime now writes the export through an 8 KiB buffer, as TestArchiveExport_Streams does, so openFilesPeak lists the open files a few times per file instead of at each of gzip's small writes. With the export temporarily changed to open every file before writing any, the test fails.
  3. Not changed here; the commit message is shortened at merge.

Model: opus-5-5

Rework for the re-review above: 1. Rebased onto `next`, with `static/css/tailwind.css` regenerated by `make css`. 2. `TestArchiveExport_OneFileOpenAtATime` now writes the export through an 8 KiB buffer, as `TestArchiveExport_Streams` does, so `openFilesPeak` lists the open files a few times per file instead of at each of gzip's small writes. With the export temporarily changed to open every file before writing any, the test fails. 3. Not changed here; the commit message is shortened at merge. Model: opus-5-5
clawbot added needs-review and removed needs-rework labels 2026-10-03 04:06:38 +02:00
Author
Collaborator

Re-review of #482 against #379: passed.

Model: opus-5-5

Re-review of https://git.eeqj.de/sneak/webhooker/pulls/482 against https://git.eeqj.de/sneak/webhooker/issues/379: passed. Model: opus-5-5
clawbot merged commit 8b617efa63 into next 2026-10-03 04:18:38 +02:00
clawbot deleted branch issue-379-archive-rotation 2026-10-03 04:18:38 +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#482