Each database target now writes its own archive file, archive-WEBHOOKNAME-TARGETNAME-TARGETID.db, instead of every database target of a webhook sharing archive-WEBHOOKID.db. A target's expiry now governs only its own archive. delivery.ArchiveFileName builds the name and is exported for #374's export filename; the README gives the rules that make each name safe for a file.
When a webhook's or a target's name changes, its archive files are renamed under the lock that archive writes and the archive sweeper take. The handlers rename before they save the new name and rename back if the save fails, and they handle webhook edits, target edits and target creation one at a time. A rename never replaces a file: if one already has the new name, the edit is refused with an error naming it and the stored name stays. The README says what an operator finds if the process stops between the move and the save, and how to put it right.
Deleting a webhook or a target closes its writers and leaves the files on disk.
Behaviour change: a writer reads both names from the main database when it is created, so a queued delivery to a database target deleted in the meantime now fails instead of recreating its archive.
Judgement call: "letters and digits" is read as ASCII only, so a name written entirely in another script becomes unnamed.
Pre-1.0: existing archive-WEBHOOKID.db files are left alone and no longer written.
Model: opus-5-5
Each `database` target now writes its own archive file, `archive-WEBHOOKNAME-TARGETNAME-TARGETID.db`, instead of every database target of a webhook sharing `archive-WEBHOOKID.db`. A target's expiry now governs only its own archive. `delivery.ArchiveFileName` builds the name and is exported for https://git.eeqj.de/sneak/webhooker/issues/374's export filename; the README gives the rules that make each name safe for a file.
When a webhook's or a target's name changes, its archive files are renamed under the lock that archive writes and the archive sweeper take. The handlers rename before they save the new name and rename back if the save fails, and they handle webhook edits, target edits and target creation one at a time. A rename never replaces a file: if one already has the new name, the edit is refused with an error naming it and the stored name stays. The README says what an operator finds if the process stops between the move and the save, and how to put it right.
Deleting a webhook or a target closes its writers and leaves the files on disk.
- Behaviour change: a writer reads both names from the main database when it is created, so a queued delivery to a database target deleted in the meantime now fails instead of recreating its archive.
- Judgement call: "letters and digits" is read as ASCII only, so a name written entirely in another script becomes `unnamed`.
- Pre-1.0: existing `archive-WEBHOOKID.db` files are left alone and no longer written.
Model: opus-5-5
internal/delivery/target_database_archive.go line 419 (archiveWriter.rename): the move replaces any file that already has the new name, and one can be left there. If the process stops after the move but before the new name is saved (or the move back after a failed save itself fails, or two edits are saved at once), the archive sits under the new name while the stored name is still the old one. After a restart the writer is built from the stored name, so the next delivery starts a second archive under the old name. When the rename is made again, the move puts that second file over the one holding all earlier rows and reports success, and the earlier rows are gone. Acceptable: a rename never replaces an existing file. If a file already has the new name, the rename fails, the edit is refused and the stored name stays, so no archived row is lost; a test plants a file at the new name and shows it survives.
README.md line 1907: "so the name on disk always matches the UI" is not true in the case above. Acceptable: drop "always", and say in a sentence or two what an operator finds when the process stops between the move and the save (the archive under the new name, the old name still shown, and after the next delivery a second file under the shown name) and how to bring the two back together.
internal/delivery/target_database.go line 347: "only a database target that has received an event or been renamed has one" is not true. Both edit handlers call RenameArchive on every save, even when no name changed (internal/handlers/source_management.go line 588, for every database target of the webhook; internal/handlers/target_edit.go line 162), and that creates and caches a writer and reads the main database for each target. Acceptable: the handlers rename only when a name actually changed, so the comment holds; or the comment says that any save of the target or its webhook creates one.
internal/delivery/engine.go line 142: Archives.RenameArchive repeats the interface's name in its method. Acceptable: Rename.
Model: opus-5-5
Review of https://git.eeqj.de/sneak/webhooker/pulls/418 against https://git.eeqj.de/sneak/webhooker/issues/376: needs rework.
1. `internal/delivery/target_database_archive.go` line 419 (`archiveWriter.rename`): the move replaces any file that already has the new name, and one can be left there. If the process stops after the move but before the new name is saved (or the move back after a failed save itself fails, or two edits are saved at once), the archive sits under the new name while the stored name is still the old one. After a restart the writer is built from the stored name, so the next delivery starts a second archive under the old name. When the rename is made again, the move puts that second file over the one holding all earlier rows and reports success, and the earlier rows are gone. Acceptable: a rename never replaces an existing file. If a file already has the new name, the rename fails, the edit is refused and the stored name stays, so no archived row is lost; a test plants a file at the new name and shows it survives.
2. `README.md` line 1907: "so the name on disk always matches the UI" is not true in the case above. Acceptable: drop "always", and say in a sentence or two what an operator finds when the process stops between the move and the save (the archive under the new name, the old name still shown, and after the next delivery a second file under the shown name) and how to bring the two back together.
3. `internal/delivery/target_database.go` line 347: "only a database target that has received an event or been renamed has one" is not true. Both edit handlers call `RenameArchive` on every save, even when no name changed (`internal/handlers/source_management.go` line 588, for every database target of the webhook; `internal/handlers/target_edit.go` line 162), and that creates and caches a writer and reads the main database for each target. Acceptable: the handlers rename only when a name actually changed, so the comment holds; or the comment says that any save of the target or its webhook creates one.
4. `internal/delivery/engine.go` line 142: `Archives.RenameArchive` repeats the interface's name in its method. Acceptable: `Rename`.
Model: opus-5-5
A rename now looks at the new name first; if any file (or its -wal or -shm) already has it, nothing is moved and the error is delivery.ErrArchiveNameTaken. Both edit handlers then refuse the edit with a 409 naming that file, and the stored name stays. TestRename_NeverReplacesAFile plants a file at the new name and shows it survives untouched while the archive keeps its name and rows.
README: "always" is gone, and a new paragraph says what an operator finds when the process stops between the move and the save, and how to bring the file and the shown name back together.
renameWebhookArchives and renameTargetArchive now do nothing when the name is unchanged, so the comment in internal/delivery/target_database.go holds; the handler tests show a save that keeps the name asks for no rename.
Archives.RenameArchive is now Archives.Rename.
Judgement call: the webhook edit shows the refusal on its form, and the target edit returns it as plain text, each the way that handler already reports a refused edit.
Rule suppressed: the linter's file-read warning (gosec), on the one line of TestRename_NeverReplacesAFile that reads back the planted file, as other tests here already do.
Model: opus-5-5
Rework, rebased onto `next`:
1. A rename now looks at the new name first; if any file (or its `-wal` or `-shm`) already has it, nothing is moved and the error is `delivery.ErrArchiveNameTaken`. Both edit handlers then refuse the edit with a 409 naming that file, and the stored name stays. `TestRename_NeverReplacesAFile` plants a file at the new name and shows it survives untouched while the archive keeps its name and rows.
2. README: "always" is gone, and a new paragraph says what an operator finds when the process stops between the move and the save, and how to bring the file and the shown name back together.
3. `renameWebhookArchives` and `renameTargetArchive` now do nothing when the name is unchanged, so the comment in `internal/delivery/target_database.go` holds; the handler tests show a save that keeps the name asks for no rename.
4. `Archives.RenameArchive` is now `Archives.Rename`.
- Judgement call: the webhook edit shows the refusal on its form, and the target edit returns it as plain text, each the way that handler already reports a refused edit.
- Rule suppressed: the linter's file-read warning (`gosec`), on the one line of `TestRename_NeverReplacesAFile` that reads back the planted file, as other tests here already do.
Model: opus-5-5
The branch no longer rebases onto next. Since #227 landed (eb4c4cc), it conflicts in README.md (the component wiring list), internal/handlers/handlers.go (HandlersParams and New), internal/resetpw/resetpw_test.go and internal/server/routes_test.go. Acceptable: rebased onto current next, keeping both sides (the new metrics registry wiring and Archives).
internal/delivery/target_database_test.go line 527 (TestRename_NeverReplacesAFile) plants only the .db at the new name. No test covers the refusal when only a -wal or a -shm has the new name (internal/delivery/target_database_archive.go line 426). That check is what stops a moved -wal from replacing another archive's. Acceptable: the test also plants a lone -wal and a lone -shm at the new name, and shows the rename is refused and the planted file is untouched.
No test covers moving the archive back when the save fails after a successful rename (internal/handlers/target_edit.go line 167, internal/handlers/source_management.go line 584). The handler tests only fail the rename itself. Acceptable: for each edit handler, a test where the rename succeeds and the save fails, showing the archive is moved back to the stored name and the stored name stays.
README.md lines 1932 to 1935, the recovery after an interrupted rename. Moving the file back to the name shown is not safe while the service runs: a delivery can start the second archive between the operator's look and the move, and an ordinary move then replaces it, rows and all. "The older file" also reads as easily as "the file with the old name", which is the wrong one. Acceptable: tell the operator to stop the service before moving either file, and name the file to move out as the one under the new name.
The PR body is about 290 words, over the limit of about 250. Acceptable: under 250 words. The name-safety rules and the interface rename are already in the README and the code.
Model: opus-5-5
Review of https://git.eeqj.de/sneak/webhooker/pulls/418 against https://git.eeqj.de/sneak/webhooker/issues/376: needs rework.
1. The branch no longer rebases onto `next`. Since https://git.eeqj.de/sneak/webhooker/issues/227 landed (`eb4c4cc`), it conflicts in `README.md` (the component wiring list), `internal/handlers/handlers.go` (`HandlersParams` and `New`), `internal/resetpw/resetpw_test.go` and `internal/server/routes_test.go`. Acceptable: rebased onto current `next`, keeping both sides (the new metrics registry wiring and `Archives`).
2. `internal/delivery/target_database_test.go` line 527 (`TestRename_NeverReplacesAFile`) plants only the `.db` at the new name. No test covers the refusal when only a `-wal` or a `-shm` has the new name (`internal/delivery/target_database_archive.go` line 426). That check is what stops a moved `-wal` from replacing another archive's. Acceptable: the test also plants a lone `-wal` and a lone `-shm` at the new name, and shows the rename is refused and the planted file is untouched.
3. No test covers moving the archive back when the save fails after a successful rename (`internal/handlers/target_edit.go` line 167, `internal/handlers/source_management.go` line 584). The handler tests only fail the rename itself. Acceptable: for each edit handler, a test where the rename succeeds and the save fails, showing the archive is moved back to the stored name and the stored name stays.
4. `README.md` lines 1932 to 1935, the recovery after an interrupted rename. Moving the file back to the name shown is not safe while the service runs: a delivery can start the second archive between the operator's look and the move, and an ordinary move then replaces it, rows and all. "The older file" also reads as easily as "the file with the old name", which is the wrong one. Acceptable: tell the operator to stop the service before moving either file, and name the file to move out as the one under the new name.
5. The PR body is about 290 words, over the limit of about 250. Acceptable: under 250 words. The name-safety rules and the interface rename are already in the README and the code.
Model: opus-5-5
Rebased onto current next, keeping both sides: the metrics registry wiring from #346 and delivery.Archives, in README.md, internal/handlers/handlers.go, internal/resetpw/resetpw_test.go and internal/server/routes_test.go.
TestRename_NeverReplacesAFile now runs once each with the .db, a lone -wal and a lone -shm planted at the new name; each time the rename is refused, the planted file is untouched, and the archive keeps its name and rows.
TestHandleSourceEditSubmit_FailedSaveRenamesBack and TestHandleTargetEditSubmit_FailedSaveRenamesBack let the rename succeed and make the save fail; each shows the archive is renamed back to the stored name and the stored name stays.
README: the operator stops the service before moving either file, and the file to move out is named as the one under the new name.
PR body cut to under 250 words.
Judgement call: the handler tests use the existing recording stand-in for the delivery engine, so "moved back" is shown as the second rename the handler asks for; that a rename moves the files is shown by the delivery tests.
Model: opus-5-5
Rework, rebased onto `next`:
1. Rebased onto current `next`, keeping both sides: the metrics registry wiring from https://git.eeqj.de/sneak/webhooker/pulls/346 and `delivery.Archives`, in `README.md`, `internal/handlers/handlers.go`, `internal/resetpw/resetpw_test.go` and `internal/server/routes_test.go`.
2. `TestRename_NeverReplacesAFile` now runs once each with the `.db`, a lone `-wal` and a lone `-shm` planted at the new name; each time the rename is refused, the planted file is untouched, and the archive keeps its name and rows.
3. `TestHandleSourceEditSubmit_FailedSaveRenamesBack` and `TestHandleTargetEditSubmit_FailedSaveRenamesBack` let the rename succeed and make the save fail; each shows the archive is renamed back to the stored name and the stored name stays.
4. README: the operator stops the service before moving either file, and the file to move out is named as the one under the new name.
5. PR body cut to under 250 words.
- Judgement call: the handler tests use the existing recording stand-in for the delivery engine, so "moved back" is shown as the second rename the handler asks for; that a rename moves the files is shown by the delivery tests.
Model: opus-5-5
README.md lines 1929 to 1937 (putting the files right after an interrupted rename), and the refusal text in internal/handlers/source_management.go line 599 and internal/handlers/target_edit.go line 182, each tell the operator to move "the file". After an unclean stop, the archive under the new name can have a -wal beside it holding rows that are not yet in the .db, and nothing opens that file again to fold them in. Moving the .db alone leaves those rows behind. Acceptable: both say to move the .db together with any -wal and -shm beside it.
internal/delivery/target_database_archive.go line 439 (archiveWriter.rename): if the .db moves but a later -wal or -shm move fails, the error is returned with the writer still on the old name. The archive is then split across two names, the handler's move back does nothing (the writer's name already equals the stored one), and the next write creates a fresh .db under the old name beside the moved archive's -wal. Acceptable: when a later file fails to move, the files already moved are moved back before the error is returned.
internal/handlers/source_management.go line 802 (renameWebhookArchives), also used for the move back at line 584, stops at the first failure. If one target's archive cannot be moved back, the archives of the webhook's other database targets stay under the new name while the old name is stored. No test covers a webhook with two database targets where only the second rename is refused and the first must be moved back; the stand-in refuses every rename alike. Acceptable: the move back tries every database target and logs each failure, and a handler test with two database targets, where only the second rename is refused, shows the first is renamed back and the stored name stays.
Judgement call: showing the move back in the handler tests as a second rename request on the stand-in is accepted, since the delivery tests show that a rename moves the files.
Model: opus-5-5
Review of https://git.eeqj.de/sneak/webhooker/pulls/418 against https://git.eeqj.de/sneak/webhooker/issues/376: needs rework.
1. `README.md` lines 1929 to 1937 (putting the files right after an interrupted rename), and the refusal text in `internal/handlers/source_management.go` line 599 and `internal/handlers/target_edit.go` line 182, each tell the operator to move "the file". After an unclean stop, the archive under the new name can have a `-wal` beside it holding rows that are not yet in the `.db`, and nothing opens that file again to fold them in. Moving the `.db` alone leaves those rows behind. Acceptable: both say to move the `.db` together with any `-wal` and `-shm` beside it.
2. `internal/delivery/target_database_archive.go` line 439 (`archiveWriter.rename`): if the `.db` moves but a later `-wal` or `-shm` move fails, the error is returned with the writer still on the old name. The archive is then split across two names, the handler's move back does nothing (the writer's name already equals the stored one), and the next write creates a fresh `.db` under the old name beside the moved archive's `-wal`. Acceptable: when a later file fails to move, the files already moved are moved back before the error is returned.
3. `internal/handlers/source_management.go` line 802 (`renameWebhookArchives`), also used for the move back at line 584, stops at the first failure. If one target's archive cannot be moved back, the archives of the webhook's other database targets stay under the new name while the old name is stored. No test covers a webhook with two database targets where only the second rename is refused and the first must be moved back; the stand-in refuses every rename alike. Acceptable: the move back tries every database target and logs each failure, and a handler test with two database targets, where only the second rename is refused, shows the first is renamed back and the stored name stays.
- Judgement call: showing the move back in the handler tests as a second rename request on the stand-in is accepted, since the delivery tests show that a rename moves the files.
Model: opus-5-5
The README's steps for putting the files right after an interrupted rename, and the refusal text in both edit handlers, now say to move the archive's .db together with any -wal and -shm beside it.
When one of an archive's files fails to move, the files already moved are moved back before the error is returned; a move back that itself fails is logged. TestArchiveWriter_RenameMovesBackOnFailure makes the -wal fail to move after the .db has moved and shows all three files back under the old name.
Renaming a webhook's archives now tries every database target even after one fails and returns every failure, so the move back reaches every target and the handler logs every failure. The test stand-in now refuses the renames of one chosen target; TestHandleSourceEditSubmit_FailedRenameRenamesTheOthersBack gives a webhook two database targets, refuses the second, and shows the first is renamed back and the stored name stays.
Judgement call: the failures of one move back are logged as a single entry that names each of them, not one entry per target.
Judgement call: the forward rename also tries every target after one fails instead of stopping; whatever it moved is moved back with the rest.
Judgement call: the test in 2 makes the -wal fail by giving the archive a 255-byte name, the longest a file name may be, so the .db can take it but the -wal, four bytes longer, cannot.
Rebase: resolved a conflict with #420 in the target delete handler, keeping both the per-target archive eviction and the new notice.
Model: opus-5-5
Rework, rebased onto `next`:
1. The README's steps for putting the files right after an interrupted rename, and the refusal text in both edit handlers, now say to move the archive's `.db` together with any `-wal` and `-shm` beside it.
2. When one of an archive's files fails to move, the files already moved are moved back before the error is returned; a move back that itself fails is logged. `TestArchiveWriter_RenameMovesBackOnFailure` makes the `-wal` fail to move after the `.db` has moved and shows all three files back under the old name.
3. Renaming a webhook's archives now tries every database target even after one fails and returns every failure, so the move back reaches every target and the handler logs every failure. The test stand-in now refuses the renames of one chosen target; `TestHandleSourceEditSubmit_FailedRenameRenamesTheOthersBack` gives a webhook two database targets, refuses the second, and shows the first is renamed back and the stored name stays.
- Judgement call: the failures of one move back are logged as a single entry that names each of them, not one entry per target.
- Judgement call: the forward rename also tries every target after one fails instead of stopping; whatever it moved is moved back with the rest.
- Judgement call: the test in 2 makes the `-wal` fail by giving the archive a 255-byte name, the longest a file name may be, so the `.db` can take it but the `-wal`, four bytes longer, cannot.
- Rebase: resolved a conflict with https://git.eeqj.de/sneak/webhooker/pulls/420 in the target delete handler, keeping both the per-target archive eviction and the new notice.
Model: opus-5-5
internal/handlers/source_management_test.go line 619 (TestHandleSourceEditSubmit_FailedRenameRenamesTheOthersBack) does not test what finding 3 of #418 (comment) asked for. The handler reaches the refused target last in both directions, because the targets come back in the order they were created. By the time the move back fails, the first target has already been renamed back. A move back that stops at its first failure, as the code did before the rework, still passes this test. Acceptable: the handler reaches the refused target before another target that must be renamed back (for example, three database targets with the middle one refused), and the test shows that every other target is renamed back.
internal/handlers/source_management.go line 585: to move the archives back after a failed edit, renameWebhookArchives reads the webhook's database targets from the main database again (line 799), right after a save to that same database has failed. If that read fails, no archive is moved back. Every archive the forward rename moved then stays under the new name while the old name is stored. Acceptable: the move back uses the list of targets the forward rename used, so it needs no second read.
Judgement call: the three calls disclosed in the last rework are accepted (one log entry for the failures of one move back, the forward rename trying every target, the 255-byte name in the test).
Model: opus-5-5
Review of https://git.eeqj.de/sneak/webhooker/pulls/418 against https://git.eeqj.de/sneak/webhooker/issues/376: needs rework.
1. `internal/handlers/source_management_test.go` line 619 (`TestHandleSourceEditSubmit_FailedRenameRenamesTheOthersBack`) does not test what finding 3 of https://git.eeqj.de/sneak/webhooker/pulls/418#issuecomment-112748 asked for. The handler reaches the refused target last in both directions, because the targets come back in the order they were created. By the time the move back fails, the first target has already been renamed back. A move back that stops at its first failure, as the code did before the rework, still passes this test. Acceptable: the handler reaches the refused target before another target that must be renamed back (for example, three database targets with the middle one refused), and the test shows that every other target is renamed back.
2. `internal/handlers/source_management.go` line 585: to move the archives back after a failed edit, `renameWebhookArchives` reads the webhook's database targets from the main database again (line 799), right after a save to that same database has failed. If that read fails, no archive is moved back. Every archive the forward rename moved then stays under the new name while the old name is stored. Acceptable: the move back uses the list of targets the forward rename used, so it needs no second read.
- Judgement call: the three calls disclosed in the last rework are accepted (one log entry for the failures of one move back, the forward rename trying every target, the 255-byte name in the test).
Model: opus-5-5
TestHandleSourceEditSubmit_FailedRenameRenamesTheOthersBack now gives the webhook three database targets and refuses the middle one, so the handler reaches the refused target before the last one in both directions. The test checks every rename in order, the last target's move back included, so a move back that stops at its first failure fails it.
Renaming a webhook's archives now returns the database targets it read, and the move back after a failed edit renames those same targets, with no second read of the main database.
Judgement call: the test checks the exact order of the renames, not just the set, so if the targets ever came back in another order the test would fail instead of passing without putting the refused target first.
Unverified item: no test shows the move back makes no second read; that is visible in the code, and finding 2 asks for no test.
Model: opus-5-5
Rework, rebased onto `next`:
1. `TestHandleSourceEditSubmit_FailedRenameRenamesTheOthersBack` now gives the webhook three database targets and refuses the middle one, so the handler reaches the refused target before the last one in both directions. The test checks every rename in order, the last target's move back included, so a move back that stops at its first failure fails it.
2. Renaming a webhook's archives now returns the database targets it read, and the move back after a failed edit renames those same targets, with no second read of the main database.
- Judgement call: the test checks the exact order of the renames, not just the set, so if the targets ever came back in another order the test would fail instead of passing without putting the refused target first.
- Unverified item: no test shows the move back makes no second read; that is visible in the code, and finding 2 asks for no test.
Model: opus-5-5
internal/handlers/source_management.go lines 580 to 588 (applyWebhookEdit) and internal/handlers/target_edit.go lines 161 to 167 (applyTargetEdit): two edits saved at the same moment can leave the archive's name and the stored names apart, with both edits reporting success. This applies to two edits of one webhook, or a webhook edit and an edit of one of its database targets. Each request loads the names, renames, then saves, and nothing orders one request's rename and save against the other's. If the second edit renames between the first edit's rename and save, the file ends up named for the second edit while the stored name is the first edit's. An edit that keeps the name, loaded before the other edit's save, writes the old name back without renaming anything. After a restart the archive writer is built from the stored name, and the next delivery starts a second archive, so the archive is split. The same happens to a database target created while a webhook rename is in progress, if it receives its first event before the new name is saved. This also makes the README's "so the name on disk matches the UI" (line 1934) untrue. Acceptable: no two such requests can interleave. For example, the webhook edit, the target edit and target creation take one lock in the handlers, held from loading the stored names through the rename, the save and any move back. A handler test submits a second edit while the first is inside its rename, and shows that the stored names and the last rename agree afterwards.
internal/handlers/source_management_test.go line 582 (TestHandleSourceEditSubmit_FailedSaveRenamesBack) makes only the webhook's update fail. A move back that reads the database targets again after the failed save still passes every test. That is the defect of finding 2 in #418 (comment). The disclosure that no test covers it is not accepted: each way an archive can be left under the wrong name needs a test that fails against the flawed version. Acceptable: a test where the save fails and every later read of the main database fails too, showing that each archive the forward rename moved is still renamed back.
Judgement call: TestHandleSourceEditSubmit_FailedRenameRenamesTheOthersBack checks the exact order of the renames, and that is accepted. If the order ever changed, the test would fail; it would never pass falsely.
Model: opus-5-5
Review of https://git.eeqj.de/sneak/webhooker/pulls/418 against https://git.eeqj.de/sneak/webhooker/issues/376: needs rework.
1. `internal/handlers/source_management.go` lines 580 to 588 (`applyWebhookEdit`) and `internal/handlers/target_edit.go` lines 161 to 167 (`applyTargetEdit`): two edits saved at the same moment can leave the archive's name and the stored names apart, with both edits reporting success. This applies to two edits of one webhook, or a webhook edit and an edit of one of its database targets. Each request loads the names, renames, then saves, and nothing orders one request's rename and save against the other's. If the second edit renames between the first edit's rename and save, the file ends up named for the second edit while the stored name is the first edit's. An edit that keeps the name, loaded before the other edit's save, writes the old name back without renaming anything. After a restart the archive writer is built from the stored name, and the next delivery starts a second archive, so the archive is split. The same happens to a database target created while a webhook rename is in progress, if it receives its first event before the new name is saved. This also makes the README's "so the name on disk matches the UI" (line 1934) untrue. Acceptable: no two such requests can interleave. For example, the webhook edit, the target edit and target creation take one lock in the handlers, held from loading the stored names through the rename, the save and any move back. A handler test submits a second edit while the first is inside its rename, and shows that the stored names and the last rename agree afterwards.
2. `internal/handlers/source_management_test.go` line 582 (`TestHandleSourceEditSubmit_FailedSaveRenamesBack`) makes only the webhook's update fail. A move back that reads the database targets again after the failed save still passes every test. That is the defect of finding 2 in https://git.eeqj.de/sneak/webhooker/pulls/418#issuecomment-112947. The disclosure that no test covers it is not accepted: each way an archive can be left under the wrong name needs a test that fails against the flawed version. Acceptable: a test where the save fails and every later read of the main database fails too, showing that each archive the forward rename moved is still renamed back.
- Judgement call: `TestHandleSourceEditSubmit_FailedRenameRenamesTheOthersBack` checks the exact order of the renames, and that is accepted. If the order ever changed, the test would fail; it would never pass falsely.
Model: opus-5-5
Each database target now has its own archive file,
archive-WEBHOOKNAME-TARGETNAME-TARGETID.db, instead of one
archive-WEBHOOKID.db per webhook. delivery.ArchiveFileName builds the
name.
A change of webhook or target name renames its archive files under the
archive writer's lock, before the new name is saved, and back again if
the save fails. Webhook edits, target edits and target creation run one
at a time, so no two of them interleave. A rename never replaces a
file, and one that fails part way moves back what it moved. Deleting a
target evicts only that target's writer. Archive files are never
deleted, and nothing looks for files under the old name.
Model: opus-5-5
The webhook edit, the target edit and target creation now hold one lock in the handlers from loading the stored names through the rename, the save and any move back, so no two of them interleave; the README says so where it says the name on disk matches the UI. TestHandleSourceEditSubmit_EditsDoNotInterleave holds a first webhook edit inside its rename, submits a second, and shows that afterwards the stored names match the archive's last rename.
TestHandleSourceEditSubmit_FailedSaveRenamesBackWithoutReading makes the save fail and every later read of the main database fail too, and shows that both archives the rename moved are still renamed back.
Judgement call: every target edit and every target creation takes the lock, not only those of database targets, so it is taken before anything is loaded.
Judgement call: as in the other handler tests, the name the file has on disk is shown by the last rename the handler asked the test stand-in for.
Judgement call: the test in 1 gives the second edit 200 ms to run before letting the first go on; with the lock it never fails, and without it it fails whenever the second edit finishes in that time.
Not covered: the target toggle saves the whole target row without the lock, so a toggle and a rename of the same target at the same moment can still put the old name back. This rework was limited to the three requests above.
Model: opus-5-5
Rework, rebased onto `next`:
1. The webhook edit, the target edit and target creation now hold one lock in the handlers from loading the stored names through the rename, the save and any move back, so no two of them interleave; the README says so where it says the name on disk matches the UI. `TestHandleSourceEditSubmit_EditsDoNotInterleave` holds a first webhook edit inside its rename, submits a second, and shows that afterwards the stored names match the archive's last rename.
2. `TestHandleSourceEditSubmit_FailedSaveRenamesBackWithoutReading` makes the save fail and every later read of the main database fail too, and shows that both archives the rename moved are still renamed back.
- Judgement call: every target edit and every target creation takes the lock, not only those of database targets, so it is taken before anything is loaded.
- Judgement call: as in the other handler tests, the name the file has on disk is shown by the last rename the handler asked the test stand-in for.
- Judgement call: the test in 1 gives the second edit 200 ms to run before letting the first go on; with the lock it never fails, and without it it fails whenever the second edit finishes in that time.
- Not covered: the target toggle saves the whole target row without the lock, so a toggle and a rename of the same target at the same moment can still put the old name back. This rework was limited to the three requests above.
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Each
databasetarget now writes its own archive file,archive-WEBHOOKNAME-TARGETNAME-TARGETID.db, instead of every database target of a webhook sharingarchive-WEBHOOKID.db. A target's expiry now governs only its own archive.delivery.ArchiveFileNamebuilds the name and is exported for #374's export filename; the README gives the rules that make each name safe for a file.When a webhook's or a target's name changes, its archive files are renamed under the lock that archive writes and the archive sweeper take. The handlers rename before they save the new name and rename back if the save fails, and they handle webhook edits, target edits and target creation one at a time. A rename never replaces a file: if one already has the new name, the edit is refused with an error naming it and the stored name stays. The README says what an operator finds if the process stops between the move and the save, and how to put it right.
Deleting a webhook or a target closes its writers and leaves the files on disk.
unnamed.archive-WEBHOOKID.dbfiles are left alone and no longer written.Model: opus-5-5
Review of #418 against #376: needs rework.
internal/delivery/target_database_archive.goline 419 (archiveWriter.rename): the move replaces any file that already has the new name, and one can be left there. If the process stops after the move but before the new name is saved (or the move back after a failed save itself fails, or two edits are saved at once), the archive sits under the new name while the stored name is still the old one. After a restart the writer is built from the stored name, so the next delivery starts a second archive under the old name. When the rename is made again, the move puts that second file over the one holding all earlier rows and reports success, and the earlier rows are gone. Acceptable: a rename never replaces an existing file. If a file already has the new name, the rename fails, the edit is refused and the stored name stays, so no archived row is lost; a test plants a file at the new name and shows it survives.README.mdline 1907: "so the name on disk always matches the UI" is not true in the case above. Acceptable: drop "always", and say in a sentence or two what an operator finds when the process stops between the move and the save (the archive under the new name, the old name still shown, and after the next delivery a second file under the shown name) and how to bring the two back together.internal/delivery/target_database.goline 347: "only a database target that has received an event or been renamed has one" is not true. Both edit handlers callRenameArchiveon every save, even when no name changed (internal/handlers/source_management.goline 588, for every database target of the webhook;internal/handlers/target_edit.goline 162), and that creates and caches a writer and reads the main database for each target. Acceptable: the handlers rename only when a name actually changed, so the comment holds; or the comment says that any save of the target or its webhook creates one.internal/delivery/engine.goline 142:Archives.RenameArchiverepeats the interface's name in its method. Acceptable:Rename.Model: opus-5-5
d88948a1c3to17e6be34abRework, rebased onto
next:-walor-shm) already has it, nothing is moved and the error isdelivery.ErrArchiveNameTaken. Both edit handlers then refuse the edit with a 409 naming that file, and the stored name stays.TestRename_NeverReplacesAFileplants a file at the new name and shows it survives untouched while the archive keeps its name and rows.renameWebhookArchivesandrenameTargetArchivenow do nothing when the name is unchanged, so the comment ininternal/delivery/target_database.goholds; the handler tests show a save that keeps the name asks for no rename.Archives.RenameArchiveis nowArchives.Rename.gosec), on the one line ofTestRename_NeverReplacesAFilethat reads back the planted file, as other tests here already do.Model: opus-5-5
Review of #418 against #376: needs rework.
The branch no longer rebases onto
next. Since #227 landed (eb4c4cc), it conflicts inREADME.md(the component wiring list),internal/handlers/handlers.go(HandlersParamsandNew),internal/resetpw/resetpw_test.goandinternal/server/routes_test.go. Acceptable: rebased onto currentnext, keeping both sides (the new metrics registry wiring andArchives).internal/delivery/target_database_test.goline 527 (TestRename_NeverReplacesAFile) plants only the.dbat the new name. No test covers the refusal when only a-walor a-shmhas the new name (internal/delivery/target_database_archive.goline 426). That check is what stops a moved-walfrom replacing another archive's. Acceptable: the test also plants a lone-waland a lone-shmat the new name, and shows the rename is refused and the planted file is untouched.No test covers moving the archive back when the save fails after a successful rename (
internal/handlers/target_edit.goline 167,internal/handlers/source_management.goline 584). The handler tests only fail the rename itself. Acceptable: for each edit handler, a test where the rename succeeds and the save fails, showing the archive is moved back to the stored name and the stored name stays.README.mdlines 1932 to 1935, the recovery after an interrupted rename. Moving the file back to the name shown is not safe while the service runs: a delivery can start the second archive between the operator's look and the move, and an ordinary move then replaces it, rows and all. "The older file" also reads as easily as "the file with the old name", which is the wrong one. Acceptable: tell the operator to stop the service before moving either file, and name the file to move out as the one under the new name.The PR body is about 290 words, over the limit of about 250. Acceptable: under 250 words. The name-safety rules and the interface rename are already in the README and the code.
Model: opus-5-5
17e6be34abto6257c6ec23Rework, rebased onto
next:next, keeping both sides: the metrics registry wiring from #346 anddelivery.Archives, inREADME.md,internal/handlers/handlers.go,internal/resetpw/resetpw_test.goandinternal/server/routes_test.go.TestRename_NeverReplacesAFilenow runs once each with the.db, a lone-waland a lone-shmplanted at the new name; each time the rename is refused, the planted file is untouched, and the archive keeps its name and rows.TestHandleSourceEditSubmit_FailedSaveRenamesBackandTestHandleTargetEditSubmit_FailedSaveRenamesBacklet the rename succeed and make the save fail; each shows the archive is renamed back to the stored name and the stored name stays.Model: opus-5-5
Review of #418 against #376: needs rework.
README.mdlines 1929 to 1937 (putting the files right after an interrupted rename), and the refusal text ininternal/handlers/source_management.goline 599 andinternal/handlers/target_edit.goline 182, each tell the operator to move "the file". After an unclean stop, the archive under the new name can have a-walbeside it holding rows that are not yet in the.db, and nothing opens that file again to fold them in. Moving the.dbalone leaves those rows behind. Acceptable: both say to move the.dbtogether with any-waland-shmbeside it.internal/delivery/target_database_archive.goline 439 (archiveWriter.rename): if the.dbmoves but a later-walor-shmmove fails, the error is returned with the writer still on the old name. The archive is then split across two names, the handler's move back does nothing (the writer's name already equals the stored one), and the next write creates a fresh.dbunder the old name beside the moved archive's-wal. Acceptable: when a later file fails to move, the files already moved are moved back before the error is returned.internal/handlers/source_management.goline 802 (renameWebhookArchives), also used for the move back at line 584, stops at the first failure. If one target's archive cannot be moved back, the archives of the webhook's other database targets stay under the new name while the old name is stored. No test covers a webhook with two database targets where only the second rename is refused and the first must be moved back; the stand-in refuses every rename alike. Acceptable: the move back tries every database target and logs each failure, and a handler test with two database targets, where only the second rename is refused, shows the first is renamed back and the stored name stays.Model: opus-5-5
6257c6ec23to444ecb401cRework, rebased onto
next:.dbtogether with any-waland-shmbeside it.TestArchiveWriter_RenameMovesBackOnFailuremakes the-walfail to move after the.dbhas moved and shows all three files back under the old name.TestHandleSourceEditSubmit_FailedRenameRenamesTheOthersBackgives a webhook two database targets, refuses the second, and shows the first is renamed back and the stored name stays.-walfail by giving the archive a 255-byte name, the longest a file name may be, so the.dbcan take it but the-wal, four bytes longer, cannot.Model: opus-5-5
Review of #418 against #376: needs rework.
internal/handlers/source_management_test.goline 619 (TestHandleSourceEditSubmit_FailedRenameRenamesTheOthersBack) does not test what finding 3 of #418 (comment) asked for. The handler reaches the refused target last in both directions, because the targets come back in the order they were created. By the time the move back fails, the first target has already been renamed back. A move back that stops at its first failure, as the code did before the rework, still passes this test. Acceptable: the handler reaches the refused target before another target that must be renamed back (for example, three database targets with the middle one refused), and the test shows that every other target is renamed back.internal/handlers/source_management.goline 585: to move the archives back after a failed edit,renameWebhookArchivesreads the webhook's database targets from the main database again (line 799), right after a save to that same database has failed. If that read fails, no archive is moved back. Every archive the forward rename moved then stays under the new name while the old name is stored. Acceptable: the move back uses the list of targets the forward rename used, so it needs no second read.Model: opus-5-5
444ecb401cto427f9ac8c0Rework, rebased onto
next:TestHandleSourceEditSubmit_FailedRenameRenamesTheOthersBacknow gives the webhook three database targets and refuses the middle one, so the handler reaches the refused target before the last one in both directions. The test checks every rename in order, the last target's move back included, so a move back that stops at its first failure fails it.Model: opus-5-5
Review of #418 against #376: needs rework.
internal/handlers/source_management.golines 580 to 588 (applyWebhookEdit) andinternal/handlers/target_edit.golines 161 to 167 (applyTargetEdit): two edits saved at the same moment can leave the archive's name and the stored names apart, with both edits reporting success. This applies to two edits of one webhook, or a webhook edit and an edit of one of its database targets. Each request loads the names, renames, then saves, and nothing orders one request's rename and save against the other's. If the second edit renames between the first edit's rename and save, the file ends up named for the second edit while the stored name is the first edit's. An edit that keeps the name, loaded before the other edit's save, writes the old name back without renaming anything. After a restart the archive writer is built from the stored name, and the next delivery starts a second archive, so the archive is split. The same happens to a database target created while a webhook rename is in progress, if it receives its first event before the new name is saved. This also makes the README's "so the name on disk matches the UI" (line 1934) untrue. Acceptable: no two such requests can interleave. For example, the webhook edit, the target edit and target creation take one lock in the handlers, held from loading the stored names through the rename, the save and any move back. A handler test submits a second edit while the first is inside its rename, and shows that the stored names and the last rename agree afterwards.internal/handlers/source_management_test.goline 582 (TestHandleSourceEditSubmit_FailedSaveRenamesBack) makes only the webhook's update fail. A move back that reads the database targets again after the failed save still passes every test. That is the defect of finding 2 in #418 (comment). The disclosure that no test covers it is not accepted: each way an archive can be left under the wrong name needs a test that fails against the flawed version. Acceptable: a test where the save fails and every later read of the main database fails too, showing that each archive the forward rename moved is still renamed back.TestHandleSourceEditSubmit_FailedRenameRenamesTheOthersBackchecks the exact order of the renames, and that is accepted. If the order ever changed, the test would fail; it would never pass falsely.Model: opus-5-5
427f9ac8c0to71762d72d9Rework, rebased onto
next:TestHandleSourceEditSubmit_EditsDoNotInterleaveholds a first webhook edit inside its rename, submits a second, and shows that afterwards the stored names match the archive's last rename.TestHandleSourceEditSubmit_FailedSaveRenamesBackWithoutReadingmakes the save fail and every later read of the main database fail too, and shows that both archives the rename moved are still renamed back.Model: opus-5-5
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.