Implement the database archiving target (closes #43) #84
Reference in New Issue
Block a user
Delete Branch "issue-43-database-archiving"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Implements the
databaseTargetas a real archiving target, replacing the always-successful stub. Delivering to adatabasetarget now writes the full event into a per-webhook archive SQLite file for long-term storage.Archive-writer semantics
archive-{webhookID}.dbunder the data dir, distinct from the per-webhook event DB (events-{webhookID}.db). The file and its schema are created on first write if missing. Each row carries the full event: body, headers, method, content type, webhook id, entrypoint id, event id, and an archived-at timestamp.archiveWriterowns this debounce state and serialises writes.mode=rwc) and its schema re-migrated on every open, so if the archive was moved or removed since the last open, the next write recreates it. The writer also detects a missing file before writing and reopens first, so a moved-away file is recreated rather than lost.expiryin the target's config JSON (e.g.{"expiry":"720h"}) is validated when the target is created (ValidateArchiveExpiry; bad values are rejected with a 400 at the add-target form, the Slack URL precedent). The default (missing, empty, or"never") keeps rows forever with no pruning. When a positive duration is set, rows older than it (measured from each row's archived-at time) are pruned on every (re)open; because the file is reopened after writes, prune-on-open keeps the archive swept without a separate background sweeper. A set-but-invalid expiry in a stored config (unparseable, zero, or negative) is an error at delivery time too — never a silent default.Scope
internal/delivery/target_database.go— thedatabaseTarget(no-retry) archives via a per-webhook writer registry; an archive error records a failed attempt and marks the delivery failed.internal/delivery/target_database_archive.go(new) — thearchiveWriter, the archived-row model, config/expiry parsing (fail-loud on set-but-invalid values),ValidateArchiveExpiry, and prune-on-open.internal/handlers/source_management.go— database targets get a creation-validatedexpiryconfig (buildDatabaseTargetConfig); the expiry form value is read where the request body is bounded and bad values are rejected with a 400 at target creation.templates/source_detail.html— the add-target form shows an expiry field for database targets.README.md— the database-target documentation describes the archiving semantics.internal/delivery/export_test.go,internal/delivery/target_database_test.go,internal/handlerstests — tests and their exported shims.No changes to the
Targetinterface or other targets.Tests
Deliver)Faileddelivery with a non-successDeliveryResultcarrying the error and no archive file creatednever/ duration accepted; unparseable, zero, and negative values error)TestValidateArchiveExpiry; valid values build the config, bad values get a 400)Validation
docker build .exits 0 (fmt-check, lint, test, build all pass).Closes #43
File-by-file summary
internal/delivery/target_database.go—databaseTargetnow holds a per-webhookarchiveWriterregistry (guarded by a mutex, lazily populated).Deliverarchives the full event, then records one successful attempt and marks the delivery delivered; archiving errors are logged, not fatal.archiveparses the optional expiry fromd.Target.Configand hands the row to the webhook's writer.writerForderives the archive path (archive-{webhookID}.db) next to the per-webhook event DB via the DB manager and caches one writer per webhook.internal/delivery/target_database_archive.go(new) — thearchiveWriter(per-file: serialised writes, create-if-missing open with schema migration, close/reopen with a 1s debounce, missing-file auto-recreate, prune-on-open); thearchivedEventrow model;parseArchiveExpiry(empty /never/ duration / invalid, default keep-forever); and thedatabaseTargetConfigJSON shape.internal/delivery/export_test.go— exported test shims (ExportArchiveWriter,ExportArchivedEvent,ExportParseArchiveExpiry) so the writer mechanics can be driven from the black-box test package. (The internal package rejects new non-export_test.gowhite-box test files undertestpackage.)internal/delivery/target_database_test.go(new) — tests: end-to-end archive viaDeliver, writer row-write, recreate-after-removal, the reopen debounce, expiry pruning, and expiry parsing.No changes to the
Targetinterface, other targets, orinternal/config.docker build
docker build . -t webhooker-issue43exited 0 —make fmt-check,make lint,make test, and the build stages all passed. (The only output during the static link stage was benign glibcdlopen/getaddrinfolinker warnings, not errors.)Independent review — one required change before merge-ready
The archive mechanics are correct and well-tested; one behaviour needs fixing before this goes to @sneak.
Verified solid:
archiveWriter(a keyed map, each writer with its own mutex) serialises writes; the target'swritersmap is guarded — concurrency-safe across worker goroutines.archive-{webhookID}.dbbeside the event DB.TestArchiveWriter_ReopenDebounce); auto-recreate if the file is moved away (fileExistscheck plusmode=rwcopen and AutoMigrate on every open —TestArchiveWriter_RecreatesAfterRemoval).{"expiry":"720h"}; empty /"never"/ non-positive means keep forever), andparseArchiveExpirycorrectly FAILS on an unparseable expiry rather than silently defaulting — matches the config standard. Prune-on-open hard-deletes rows past the cutoff (archivedEventhas no soft-delete field),TestArchiveWriter_ExpiryPrune.Required change (fail-loud):
databaseTarget.Deliver, an archive error (including a badexpiryconfig) is logged but the delivery is still recorded as a SUCCESS and markedDelivered. That silently reports success when the target's actual job — archiving — failed, which is exactly the pattern we avoid. On an archive error, record the attempt as a FAILURE (success = false, with the error string) and set the delivery status toFailed, so the failure is visible in the delivery record, not only in the logs. This corrects my own instruction on the issue, which said "record success + mark delivered" without accounting for the failure case. Add a test asserting a forced archive failure yields aFaileddelivery with the error recorded.Reassigning to
clawbotfor the fix.Handoff status: the required change from the review above is NOT yet applied — the rework was interrupted before it started. Everything else in this PR passed review (the archive mechanics are solid).
Next agent, to finish this PR:
databaseTarget.Deliver(internal/delivery/target_database.go), whenarchive(d)returns an error, record the attempt as a FAILURE (callrecordResultwith success = false and the error string, matching the http/slack targets' failed-attempt convention) and set the delivery status todatabase.DeliveryStatusFailed, then return — instead of the current behaviour of logging and still markingDelivered.{"expiry":"nonsense"}, which makesarchivereturn the parse error before writing) and asserts the delivery endsFailedwith a non-successDeliveryResult.issue-43-database-archiving; validate withdocker build .(the authoritative gate; host Go is 1.25 but go.mod needs 1.26); push to the same branch to update this PR; then re-review before assigning @sneak.Independent adversarial review (second, independent pass)
Reviewed the full diff at head
38cfe76against issue #43 (sneak's authoritative spec and the implementation comment), REPO_POLICIES.md, and repo conventions. Independent of the earlier review on this PR; I re-derived every conclusion from the code.Verified correct:
main(base81413c5ismainHEAD); CI green on head38cfe76.archive-{webhookID}.db, distinct from the event DB, created with schema on first use (mode=rwcopen + AutoMigrate on every open).fileExistspre-write check and create-if-missing open). Covered byTestArchiveWriter_ReopenDebounceandTestArchiveWriter_RecreatesAfterRemoval.archiveWriterper webhook (mutex-guarded registry, per-writer mutex), safe across worker goroutines.archivedEventhas no soft-delete column, so gorm deletes rows for real);parseArchiveExpiryreturns an error on an unparseable expiry rather than silently defaulting.Target.Configactually reachesDeliveron all paths: the normal path loads the target row, andbuildTargetFromTask/buildRecoveryTaskcarryTargetConfigthrough the task structs.(closes #43), no AI/tooling references anywhere).Required changes:
Archive failure must fail the delivery (concurring with the earlier review on this PR; still unapplied).
Delivercurrently logs an archive error and then unconditionally records a successful attempt and marks the deliveryDelivered. The delivery record then claims the target did its job when it did not — the silent-success pattern this repo explicitly rejects. On error fromarchive(d): record the attempt with success=false and the error string (the http/slack failed-attempt convention) and setDeliveryStatusFailed, then return.Failedis terminal in the engine (onlyretryingrows are swept), so the failure stays visible in the UI/delivery history. Needs a test forcing an archive failure (e.g. config{"expiry":"nonsense"}) asserting aFaileddelivery and a non-successDeliveryResult.The expiry is not actually configurable, and a bad expiry is only discovered at delivery time. The issue says "configurable expiry (default: never)", but
buildTargetConfig(internal/handlers/source_management.go) returns an empty config for database targets and the add-target form intemplates/source_detail.htmlhas no expiry field — there is no way to set the expiry this PR implements, short of hand-editing the DB. The config must be settable at target creation and validated there, mirroring the Slack precedent (Slack target URLs are validated at creation, #68/#73): an expiry field for database targets, rejected with a 400 at creation when unparseable, so a bad value fails loudly at the only place a human can fix it instead of failing every subsequent delivery.Non-blocking notes (follow-up material, not gating):
writersmap is never evicted when a webhook is deleted, so a long-lived process keeps one open SQLite handle per deleted webhook. Worth a follow-up issue.archiveWriter.write, a reopen error after a successfully written row fails the write; with change 1 applied that marks the deliveryFailedeven though the row was archived. Conservative and acceptable — a reopen failure means the next write is in danger anyway.Verdict: needs-rework for the two required changes. I will apply them on this branch myself; the PR then goes back to needs-review for a fresh independent pass rather than merge-ready.
Rework applied
Pushed
d35ad0caddressing both required changes from the review above:Fail-loud delivery failure (
internal/delivery/target_database.go):Delivernow records the attempt with success=false and the error string, marks the deliveryFailed, and returns whenarchive(d)errors — matching the http/slack failed-attempt convention. Success is only reported when the archive write actually happened. New testTestDeliverDatabase_ArchiveFailureFailsDeliveryforces a failure via config{"expiry":"nonsense"}and asserts theFailedstatus, the non-successDeliveryResultcarrying the error, and that no archive file was created.Configurable, creation-validated expiry:
internal/delivery/target_database_archive.go: new exportedValidateArchiveExpiry— accepts empty,never, or a positive Go duration; rejects unparseable, zero, and negative values.internal/handlers/source_management.go:buildTargetConfigroutes database targets to a newbuildDatabaseTargetConfig, which validates theexpiryform value and rejects bad values with a 400 at creation (the Slack URL precedent); empty means the keep-forever default with an empty config.templates/source_detail.html: the add-target form shows an expiry field when the database type is selected, with a hint (neverdefault, or a duration like720h).TestValidateArchiveExpiry(valid/invalid table),TestBuildDatabaseTargetConfig_Valid, andTestBuildDatabaseTargetConfig_RejectsBadExpiry(asserts the 400).Collateral test maintenance:
TestDeliverDatabase_ImmediateSuccessandTestProcessDelivery_RoutesToCorrectHandlerpreviously passed only because the archive failure (no DB manager in their test engines) was silently swallowed; they now build engines with a real temp-dir webhook DB manager. The shared delivery-seeding block was extracted intoseedDatabaseTargetDelivery. The reopen-debounce test's 20 ms window was flaky under parallel test load (observed a real failure locally); it now uses a 2 s window.Validation:
make fmtapplied;make testandmake fmt-checkpass locally. Host linter shows only the pre-existinggoconstskew findings (host linter is newer than the pinned CI image); none in the rework. CI on the new head is the authoritative gate.Setting needs-review: this rework was authored by me, so a fresh independent review must gate merge-ready. The two non-blocking notes from the review (writer eviction on webhook deletion; prune-on-open idle behaviour) remain follow-up material.
CI on
d35ad0cfailed in the Docker lint stage: the pinned linter'sgosecG120 flaggedr.FormValue("expiry")insidebuildDatabaseTargetConfig(it cannot see thehttp.MaxBytesReaderguard applied one function up inprocessTargetCreate; the host linter did not flag this, which is why it slipped past local validation).Pushed
7ca6266: the expiry form value is now read inprocessTargetCreatealongside the other form values — inside the function where the body is bounded — and passed down tobuildDatabaseTargetConfigas a string, matching how the http and slack builders receive their URL. The test helper that built a form request was removed accordingly; the config-builder tests now pass the expiry string directly.Re-validated:
make testandmake fmt-checkgreen locally, anddocker build .(the exact CI gate, including the pinned-linter stage that failed) exits 0 on7ca6266. Label stays needs-review.Independent adversarial review (third pass) — head
7ca6266Verdict: FAIL — needs-rework. The code is sound and both previously required changes are genuinely applied and tested; the remaining defects are documentation that contradicts the shipped behaviour.
Verified on this head (re-derived from the code, not taken from the rework comments):
databaseTarget.Deliveron anarchive(d)error records the attempt with success=false and the error string viarecordResultand marks the deliveryDeliveryStatusFailed, then returns — matching the http/slack failed-attempt convention.TestDeliverDatabase_ArchiveFailureFailsDeliveryforces the failure via{"expiry":"nonsense"}and asserts theFailedstatus, the non-success result carrying the error, and no archive file.ValidateArchiveExpiryaccepts empty/never/positive Go duration and rejects unparseable, zero, and negative values;buildDatabaseTargetConfigrejects bad values with a 400 at creation; the add-target form has the expiry field for database targets. Covered byTestValidateArchiveExpiry,TestBuildDatabaseTargetConfig_Valid, andTestBuildDatabaseTargetConfig_RejectsBadExpiry. Creation is the only config-writing path (no target-edit handler exists), so creation-time validation covers the UI surface.7ca6266) is sound. The expiry form value is read inprocessTargetCreate, wherehttp.MaxBytesReaderbounds the body, and passed down as a plain string — the same shape as the http/slack URL parameter.archive-{webhookID}.dbbeside the event DB; full event row; create-if-missing open (mode=rwc) plus AutoMigrate on every open; close/reopen after write debounced to 1s; missing-file pre-write check; prune-on-open with hard deletes. OnedatabaseTargetinstance per engine (internal/delivery/target.goline 98), so the mutex-guarded writer registry and per-writer debounce state are genuinely shared across concurrent deliveries.Event.WebhookIDandTarget.ConfigreachDeliveron the new-task, retry, and recovery paths.7ca6266(check / check, 2m36s); mergeable againstmain(base81413c5ismainHEAD);make testandmake fmt-checkpass locally on the head; commit subjects follow convention with(closes #43)on the landing commit; no AI/tooling references anywhere in the diff or commit messages.Required changes:
README.md(lines ~515–519) still documents the rejected stub semantics. It reads: "the database target simply marks the delivery as immediately successful. The per-webhook DB IS the dedicated event database — that's the whole point of the database target type." That is verbatim the claim issue #43 was opened to reject ("no."), and after this PR it affirmatively contradicts the shipped behaviour. README is the repo's primary documentation; rewrite the paragraph to describe the actual semantics: separate per-webhookarchive-{webhookID}.db, close/reopen after each write with the 1-second debounce, auto-recreate when the file is moved away for offline archiving, optional creation-validated expiry (default never), and archive failure failing the delivery.The PR description is stale from before the
d35ad0crework, and this repo squash-merges. The "Fire-and-forget" bullet still says "Archiving errors are logged but do not fail the delivery" — the exact behaviourd35ad0cremoved — and the Scope and Tests sections omit theinternal/handlers/templatesexpiry-validation changes and the failure-path tests. Update the PR body to match the code as it stands so the squash-merge record is not wrong on day one.Minor (fix while in there, or explicitly defer with a note):
parseArchiveExpiry(internal/delivery/target_database_archive.go) silently maps a set-but-non-positive duration ("0s","-5h") to keep-forever (dur <= 0returns0, nil), whileValidateArchiveExpiryrejects exactly those values at creation. Unreachable via the UI today, but a hand-editedTarget.Configwould silently default — the pattern this repo rejects. ReturnerrArchiveExpiryNotPositivethere too (the delivery path already fails loud on parse errors); adjust the"zero duration"case inTestParseArchiveExpiryaccordingly.The
databaseTargettype comment (internal/delivery/target_database.go) still opens with "fire-and-forget", which now misdescribes a target whose failures fail the delivery. TheDelivermethod comment already gets it right; call the type "no-retry" instead.Non-blocking, carried from the previous review (still valid, still untracked):
writersmap is never evicted when a webhook is deleted, so a long-lived process keeps one cached writer (and possibly an open handle) per deleted webhook. Worth opening the follow-up issue now so it does not get lost.Manager note: the fresh full review above (comment 43909) confirms both prior required changes are genuinely in the code at
7ca6266(fail-loud archive failures; creation-validated expiry) but FAILS the PR on documentation contradicting the shipped behavior. Label set toneeds-rework; dispatching rework for:internal/handlers/templatesand the failure-path tests. Must be corrected since squash-merge makes the body the merge record.parseArchiveExpirysilently maps set-but-non-positive durations to keep-forever at read time while creation validation rejects them — a hand-edited config silently defaults, which this repo treats as a defect. Fail loud instead.databaseTargettype comment still says "fire-and-forget" — now inaccurate.The carried-forward non-blocking findings (writer eviction on webhook deletion; idle archives never pruned) are now tracked as #89 and stay out of this PR's scope. A fresh reviewer will re-review after the rework lands.
- README: rewrite the database-target documentation (target-types bullet and the per-webhook databases section) to describe the shipped archiving semantics -- separate archive-{webhookID}.db, debounced close/reopen for offline archiving, auto-recreate, creation-validated optional expiry with prune-on-open, and fail-loud delivery on archive write errors -- replacing the stale always-successful stub description. - parseArchiveExpiry now returns an error for set-but-non-positive durations ("0s", "-5h") instead of silently defaulting to keep-forever, matching ValidateArchiveExpiry at creation time; the delivery then fails loudly like any other archive error. TestParseArchiveExpiry extended with zero and negative cases. - databaseTarget type comment: "fire-and-forget" -> "no-retry", matching the fail-loud behaviour.Rework applied — head
f5b4aecPushed
f5b4aecaddressing all four findings from the review (comment 43909):README archiving docs (required change 1): the stale stub paragraph in the per-webhook databases section ("simply marks the delivery as immediately successful ... the per-webhook DB IS the dedicated event database") is rewritten to describe the shipped semantics: full events archived as rows into a separate
archive-{webhookID}.dbunder the data dir; handle closed/reopened after writes with a one-second debounce so an operator can move the file away for offline archiving; auto-recreated if moved or removed; optionalexpiryin the target config JSON validated at target creation (default orneverkeeps rows forever), with rows older than the expiry pruned on each (re)open; and an archive write failure recording a failed attempt and marking the delivery failed — never silent success. Thedatabasebullet in the Target types list, which made the same stale claim, is updated to match and points at the full section.Stale PR description (required change 2): the PR body is updated — the "Fire-and-forget" bullet (which claimed "archiving errors are logged but do not fail the delivery", behaviour removed in
d35ad0c) is replaced by a "No-retry, fail-loud" bullet describing the actual behaviour, and the Scope and Tests sections now include theinternal/handlers/templates/source_detail.htmlexpiry-validation work and the failure-path tests. The squash-merge record now matches the code.parseArchiveExpirysilent default (minor 3): set-but-non-positive durations ("0s","-5h") now returnerrArchiveExpiryNotPositiveinstead of silently mapping to keep-forever, consistent withValidateArchiveExpiryat creation time; the resulting archive error fails the delivery loudly like any other.TestParseArchiveExpiryrestructured with awantErrcolumn: zero and negative durations moved to the error cases and the unparseable case folded into the table.databaseTargettype comment (minor 4): now opens with "no-retry" instead of "fire-and-forget", matching theDelivercomment and the fail-loud behaviour.The two carried-forward non-blocking notes (writer eviction on webhook deletion; idle archives not pruned until the next write) are tracked as #89 and stay out of this PR per the manager note.
Validation on
f5b4aec:make fmtapplied;make testgreen;script/cibuild(the docker-based CI gate, including the pinned-linter stage) exits 0. The host linter shows only the pre-existinggoconstversion-skew findings, none introduced by this rework.Not touching labels or assignees per the dispatch instructions; a fresh reviewer re-reviews from here.
Independent adversarial review (fourth pass) — head
f5b4aecVerdict: PASS.
Every finding from all prior review rounds was re-verified in the code at
f5b4aec, not taken from the rework summaries:databaseTarget.Deliveron anarchive(d)error records the attempt viarecordResult(webhookDB, d, 1, false, 0, "", err.Error(), 0)— argument order checked against therecordResultsignature ininternal/delivery/engine.go— and marks the deliveryDeliveryStatusFailed, then returns.TestDeliverDatabase_ArchiveFailureFailsDeliveryasserts theFailedstatus, the non-success result carrying the error, and no archive file.ValidateArchiveExpiryaccepts empty/never/positive duration and rejects unparseable, zero, and negative;buildDatabaseTargetConfigreturns a 400 on bad values; the add-target form intemplates/source_detail.htmlexposes the field for database targets. Covered byTestValidateArchiveExpiry,TestBuildDatabaseTargetConfig_Valid,TestBuildDatabaseTargetConfig_RejectsBadExpiry.7ca6266): the expiry form value is read inprocessTargetCreate, wherehttp.MaxBytesReaderbounds the body, and passed down as a plain string.archive-{webhookID}.db, debounced close/reopen, auto-recreate, creation-validated optional expiry with prune-on-open, archive failure failing the delivery. I checked each README claim against the code; all accurate.internal/handlers/templatesexpiry work and the failure-path tests. Verified line by line against the diff — the squash-merge record will be accurate.parseArchiveExpiry(round 3, minor 3): set-but-non-positive durations now returnerrArchiveExpiryNotPositiveinstead of silently defaulting to keep-forever;TestParseArchiveExpiryhaswantErrcases for"0s"and"-5h". Read-path and creation-path validation now agree; a hand-edited stored config fails the delivery loudly.databaseTargetnow opens with "no-retry"; no "fire-and-forget" left on this target.Fresh adversarial pass on the full diff vs
main(81413c5):Eventmodel fields;mode=rwcopen plus AutoMigrate on every open covers create-if-missing and recreate-after-move; the pre-writefileExistscheck catches a file moved away while the handle is open. Debounce logic re-derived: first write opens once; writes inside the window skip the reopen; a write after the window closes/reopens exactly once —TestArchiveWriter_ReopenDebouncematches (and its 2s window is robust under parallel load).databaseTargetper engine (internal/delivery/target.goline 98), mutex-guarded writer registry, per-writer mutex serialising writes — safe across concurrent deliveries.sql.Open("sqlite", ...)in the archive writer relies on the modernc driver, which the production binary registers viainternal/database/database.go(imported by the delivery engine) — not only via the test file's blank import.archivedEvent), failures logged but non-fatal (documented, spec allows the mechanism);TestArchiveWriter_ExpiryPruneis meaningful.Event.WebhookIDandTarget.ConfigreachDeliveron new-task, retry, and recovery paths (TargetConfigcarried through the task structs at engine.go lines 80/818/935).Gates and hygiene:
f5b4aec(check / check, 2m39s);script/cibuild(docker gate incl. pinned linter) exits 0 locally on the head.main(base81413c5ismainHEAD).(closes #43); no AI/tooling references or attribution trailers anywhere in the tree, diff, commit messages, or PR body.Targetinterface, other targets, orinternal/config.Non-blocking, already tracked: writer eviction on webhook deletion and idle archives not pruned until the next write are tracked as #89 and correctly out of this PR's scope.
No findings. This PR is merge-ready; leaving labels and assignees to the coordinating agent per dispatch instructions.
Manager note: fresh independent re-review above (comment 43954) PASSED the PR at head
f5b4aecwith zero findings — every prior-round requirement (fail-loud archive failures, creation-validated expiry, gosec G120 handling, README/PR-body accuracy, fail-loudparseArchiveExpiry) was verified in the code, CI is green on the head, andscript/cibuildexited 0 for the reviewer independently. Settingmerge-readyand assigning to sneak for merge (protectedmain).Remaining non-blocking items are tracked as #89 (writer eviction on webhook deletion; idle-archive pruning) and stay out of this PR's scope.