Recommendations for a clearer, simpler object and data model #499

Open
opened 2026-10-03 14:31:04 +02:00 by clawbot · 0 comments
Collaborator

Reviewed next at fe5e0d41733617022fb3a7b003c3c2b118059111 (2026-10-03, "Install ESLint and prettier with yarn 4 through corepack"). Read in full: every non-test Go file in internal/database, in internal/delivery except ssrf.go, in internal/handlers except auth.go, profile.go, settings.go, index.go, healthcheck.go and metrics.go, in internal/server and cmd/webhooker; README.md and TODO.md; the templates for the words they show; and the tracker history behind the names (#3, #12, #16, #85, #206, #274, #316, #399, #477). Not covered, because they are the machinery around the model rather than the model: config, middleware, session, metrics, logger, gormlog, logfield, lifecycle, datadir, resetpw, reqtls, banner, the SSRF guard's internals, the scripts, the JavaScript and the tests. Nothing was built or run and no file in the repo was touched; every claim below was checked by reading the tree at that commit, and confidence is high for every item unless the item says otherwise.

Items, most valuable first. Each gives the change, where it applies, why it makes the model clearer or simpler, its rough size, and what it depends on.

  • Make a delivery task carry only identifiers, and read the event and target rows when the attempt runs.

    • Where: internal/delivery/engine.go (Task, buildEventFromTask, buildTargetFromTask, buildRecoveryTask, hydrateEvent, MaxInlineBodySize), internal/handlers/webhook.go (buildDeliveryTasks, inlineBody), internal/handlers/delivery_replay.go (createReplayDelivery, replayBody), the README's "Request Flow" and "Dependency Injection" sections.
    • Why: the README still promises that a task carries everything "so the engine can deliver without reading from any database", but every task now reads the delivery row (loadDelivery) and the event row (hydrateEvent, for created_at), and a retry reads the target row as well (abandonRetryForMissingTarget). The copies buy nothing and cost a 14-field struct built in three places and turned back into model structs in two more. They also carry a wrong value: a retry chain keeps delivering with the target's settings as they were when the chain began (the comment on abandonRetryForMissingTarget says so), so an edit made during a backoff is ignored until a restart, while replay deliberately uses the current settings. With a task of delivery ID and webhook ID (the attempt number is the count of recorded attempts plus one, which recovery already computes), every attempt reads the current target, no body sits in a 10,000-deep channel, and the three builders become one line each.
    • Size: medium. Depends on nothing; it shrinks the next two items.
  • Drop the relation fields that point across the two database files.

    • Where: internal/database/model_event.go (Event.Webhook, Event.Entrypoint), model_delivery.go (Delivery.Target, and Delivery.Event, which is only ever filled in memory), model_target.go (Target.Deliveries), event_db_isolation.go (omitAssociations, purgeTargetRows, eventDBSweptVersion), webhook_db_manager.go (openDB), the d.Event and d.Target reads in internal/delivery/target_*.go, and Omit(clause.Associations) in internal/handlers/delivery_replay.go.
    • Why: the configuration tier and the event tier live in different files, so these relations can never be loaded with Preload, yet GORM treats them as real. AutoMigrate adds the parent tables to every events-*.db: targets, confirmed by the sweep that exists for it and by TestEventDBHoldsNoTargetRows, and webhooks, entrypoints and users by the same rule (verified in GORM's migrator.go, ReorderModels with autoAdd). It declares foreign keys to those empty tables, inert only because SQLiteDSN never turns foreign_keys on; turned on, every delivery insert would fail. And the association save copied target rows, credentials included, into those files (#206). The guard callback, the purge-and-VACUUM sweep with its user_version stamp, and the Omit at the replay site all exist to fight relations nothing uses; the purge is also upgrade-path code for files written by an earlier build, which the project has ruled out. Targets then receive the event and the target as plain arguments to Deliver, which is all they read from d.Event and d.Target today.
    • Size: medium. Depends on the task item above, or can precede it by passing the two values separately.
  • Remove soft delete from the event tier.

    • Where: internal/database/base_model.go (one base with DeletedAt for the configuration tier, one without for events, deliveries and attempts), the repeated DeletedAt fields and index tags in model_event.go, model_delivery.go and model_delivery_result.go, the Unscoped() calls in retention.go, the raw deleted_at IS NULL in internal/handlers/event_body.go and event_log.go, the README's "Event-tier indexes" and "Common Fields" sections.
    • Why: nothing ever soft-deletes an event, a delivery or an attempt; the only deletes in the event tier are retention's hard deletes. Yet every event-tier query carries deleted_at IS NULL, six of the seven event-tier indexes include deleted_at only to keep SQLite off the deleted_at index, the models repeat the field to put it in those indexes, and the README needs a paragraph on column order. Without the column the indexes are the plain ones a reader expects (status, finished_at, target_id; event_id; delivery_id; created_at; resubmitted_from_id; entrypoint_id, resubmitted_from_id, created_at) and the Unscoped calls and raw predicates go. UpdatedAt is likewise never read on events or attempts and can go with it; deliveries keep it for the pending sweep.
    • Size: small to medium. Depends on nothing.
  • Store a target's settings as columns instead of a per-type JSON string.

    • Where: internal/database/model_target.go (Config), internal/delivery/target_http.go (HTTPTargetConfig, parseHTTPConfig), target_slack.go (SlackTargetConfig, parseSlackConfig), target_database_archive.go (databaseTargetConfig, parseArchiveExpiry), target_database_rotation.go (parseArchiveRotation), target_config_view.go, target_config_edit.go, target_redact.go (targetSecrets), internal/handlers/target_create.go (buildTargetConfig, its three builders, marshalTargetConfig), target_edit.go (configUnreadableMessage), the README's "Target" section.
    • Why: a target has six settings in all (destination URL, extra headers, timeout, attempts, archive expiry, archive rotation). Keeping four of them in a JSON string whose shape depends on Type means every reader parses first and every reader carries a "configuration could not be read" branch (the view, the edit form, the delivery, the archive sweep, the redactor), the forms encode, the two URL-bearing types spell the same thing differently (url, webhookUrl), the credential lives in a string that must be tagged json:"-" and masked as a unit, and MaxRetries is the one setting that lives outside the string. As columns (URL, Headers, TimeoutSeconds, MaxAttempts, ArchiveExpiry, ArchiveRotation; the ones a type does not use stay empty) the struct says what a target can have, validation happens once at the form, masking is per field, the view types can live with the handlers that render them, and encryption at rest (#212), if it ever comes, covers two named columns instead of a blob.
    • Size: large. Depends on nothing; the attempts item below folds into it.
  • One recovery routine, run at start and on every tick.

    • Where: internal/delivery/engine.go: recoverInFlight, recoverWebhookDeliveries, recoverRetryingDeliveries, recoverSingleRetry and recoverPendingDeliveries on one side; retrySweep, sweepOrphanedRetries, sweepWebhookRetries, sweepSingleRetry and sweepWebhookPending on the other; target.go (rescheduler: remainingBackoff, backoffElapsed) and their implementations in target_http.go.
    • Why: the start-up pass and the 60-second sweep do the same job (list the webhooks, list each one's retrying and pending deliveries, settle those that already have a successful attempt, hand the rest to takeForRedispatch) in two parallel sets of functions. The only differences are the 15-minute age bound on pending rows and whether a retrying row goes to a timer for remainingBackoff or straight to the channel once backoffElapsed; a timer with a zero delay is "now", so one routine with a minimum-age parameter (zero at start) covers both and backoffElapsed goes. A reader then learns one recovery path. This keeps recovery engine-owned, which is also the recommendation recorded on #85.
    • Size: medium. Depends on nothing; smaller after the task item.
  • Move "store the event, create its deliveries, queue them" from the handlers into the delivery engine.

    • Where: internal/handlers/webhook.go (eventSource, createAndFanOut, buildDeliveryTasks), event_resubmit.go (queueResubmit), delivery_replay.go (createReplayDelivery), internal/delivery/engine.go (Notifier, Archives, CircuitBreakers) and the three bridge functions in cmd/webhooker/main.go that provide them.
    • Why: the write path that defines the product (one event row, one pending delivery per active target and the totals, in one transaction, then queue) lives in the HTTP package and is spelled twice (fan-out in createAndFanOut, one delivery in createReplayDelivery), while the engine, which owns deliveries everywhere else, learns of them through a Notifier carrying copies. With two methods on the engine (store an event and queue its deliveries; queue one more delivery of a stored event), the receiver, resubmit and replay handlers become request parsing plus one call, the handlers depend on one *delivery.Engine instead of three interfaces and three bridges, and the totals bookkeeping has one home.
    • Size: medium. Depends on the task item.
  • Give the archive files their own package.

    • Where: internal/delivery/target_database.go, target_database_archive.go, target_database_rotation.go, target_database_export.go and archive_sweeper.go (about 2,000 of the package's 7,500 non-test lines), Handlers.renameMu and the Archives interface, internal/handlers/target_download.go and target_list.go (archiveFileView).
    • Why: the delivery package is "deliver an event to a target", but a quarter of it is file management for one target type (naming, rotation, close and reopen, pruning, rename with rollback, export, the idle sweep), and the lock that orders renames against writes and exports lives in the HTTP handlers struct and is handed into delivery.NewArchiveExport. In a package of its own (internal/archive, say) the writers, the sweeper, the export and the one lock live together, the archive target type becomes a few lines that call it, and the handlers call it for rename, download and the row's file figures.
    • Size: medium, mostly moving code. Depends on nothing; pairs with the database to archive rename below.
  • Store retention as a plain day count, 0 meaning forever.

    • Where: internal/database/model_webhook.go (RetentionForeverDays, BeforeSave, retainsForever, the gorm:"default:30" tag), retention.go (retentionCutoff), internal/handlers/shared.go (parseRetentionDays), the README's "Webhook" section.
    • Why: the 365000-day sentinel exists only because the column default of 30 turns a stored 0 back into 30. Apply the default in the create form (it already falls back to DefaultRetentionDays) and drop the column default, and a 0 is stored as 0: no sentinel, no BeforeSave rewrite, no three bands of accepted values to document, and the edit form shows 0 for forever, which is what its copy already says. MaxFiniteRetentionDays stays.
    • Size: small. Depends on nothing.
  • Count delivery attempts rather than retries, with a minimum of one.

    • Where: internal/database/model_target.go (MaxRetries), internal/handlers/target_retries.go, internal/delivery/target_http.go (httpCore.deliver, handleRetry), target_config_view.go (maxRetriesField), the README's "Target" section.
    • Why: today 0 and 1 both mean one attempt (0 without the circuit breaker, 1 with it), the UI shows both as "1" and explains one of them in words, and the README spends two paragraphs on the difference; the name is the confusion #316 and #399 recorded. Store the number of attempts, at least 1: one attempt is fire-and-forget, more than one retries with the breaker, and the form's default is 1.
    • Size: small. Depends on nothing; folds into the columns item if that is done.
  • Delete the APIKey model and the empty /api/v1 route group.

    • Where: internal/database/model_apikey.go, model_user.go (User.APIKeys), models.go, internal/server/routes.go (/api/v1), the README's "APIKey", "API (Planned)" and "Authentication" entries.
    • Why: nothing reads or writes the model (its only references are the migration list and the User relation), so it is a table, a relation and a documented entity for a feature that does not exist. TODO.md lists the API as a future step, and the model can return with it.
    • Size: small. Depends on nothing.
  • Decide whether lifetime figures must outlive retention; if not, drop the three totals tables.

    • Where: internal/database/model_totals.go; the AddEventTotals, AddTargetTotals and AddEntrypointTotals calls in internal/handlers/webhook.go, delivery_replay.go, internal/delivery/engine.go (writeDeliveryStatus) and internal/database/retention.go (deleteEvents); the readers in internal/handlers/webhook_stats.go, webhook_list.go, target_list.go and entrypoint_view.go.
    • Why: three counter tables, kept in step from four files, exist so the statistics pane can show counts from before retention deleted the rows, and last-event times without a scan. If the figures within retention are enough (the pane shows both today), the tables and their bookkeeping go and every figure is a count over an index. What is given up: the lifetime counts, and an entrypoint's last-event time once retention has removed that event. This is a product question for sneak, not a defect; confidence here is about the cost, not about the answer.
    • Size: medium. Depends on nothing.

Names. Each line gives the current name, the proposed name and the reason. A proposed word that the repo's documents do not already use is marked "new word" for sneak to approve.

  • HandleWebhook, handlers/webhook.go, processWebhookRequest, readWebhookBody, finishWebhookResponse, maxWebhookBodySize, setupWebhookRoutes, and the log lines "webhook request received" and "webhook event created": HandleReceive, handlers/receiver.go, processReceivedRequest, readEventBody, finishReceiverResponse, maxEventBodySize, setupReceiverRoutes, "event received", "event stored". The README calls /h/{uuid} the receiver; "webhook" then means only the configured thing, as #12 decided.
  • WebhookDBManager (and dbMgr, WebhookDBMgr, DBManager): EventDBManager. The README calls the files per-webhook event databases and the code already says "event database" in its errors; "webhook DB" reads as the webhooks table.
  • database.Database: MainDB. The README and MainDBFileName call it the main database; database.Database names nothing.
  • Entrypoint.Path: Entrypoint.Secret (new word as a field name). It stores a bare UUID, not a path (the README: "The /h/ prefix is route only and is not stored"), and the README calls the URL the authentication secret.
  • TargetTypeDatabase and the stored value database, databaseTarget, databaseTargetConfig, buildDatabaseTargetConfig, target_database*.go: TargetTypeArchive and archive, archiveTarget, archiveConfig, buildArchiveConfig, target_archive*.go. The UI already calls the type Archive (#399), and "database" otherwise means the main database, an event database or an archive file.
  • Target.MaxRetries, the form field max_retries, parseMaxRetries, maxTargetRetries: MaxAttempts, max_attempts, parseMaxAttempts, maxTargetAttempts. It holds attempts in all, as the UI label "Delivery attempts" says.
  • DeliveryResult, the table delivery_results, AttemptNum, Duration: DeliveryAttempt, delivery_attempts, Number, DurationMS. The UI, the README and the handlers call these attempts (delivery_attempts.html, "Attempt 3", loadDeliveryResults returns attempts), and the view already says DurationMS.
  • SlackTargetConfig.WebhookURL, the key webhookUrl, errMissingWebhookURL: URL, url, errMissingTargetURL (shared with the http type). The form field is url for both types and TargetConfigForm maps both to URL; one meaning of "webhook" fewer. Moot if the columns item lands.
  • HandleSourceList, HandleSourceCreate, HandleSourceCreateSubmit, HandleSourceDetail, HandleSourceEdit, HandleSourceEditSubmit, HandleSourceDelete, renderSourceDetail, setupSourceRoutes, the route parameter sourceID, sourceFormInput, newSourceFormData; the templates sources_list.html, sources_new.html, source_detail.html, source_edit.html; the tests source_*_test.go: the same names with Webhook or webhook (HandleWebhookList, webhookID, webhook_list.html and so on). The entity was renamed in #12; these kept the old name. HandleSourceLogs and source_logs.html are the event log: HandleEventLog, event_log.html.
  • The routes /hooks, /hooks/new, /hook/{id}/...: /webhooks, /webhooks/new, /webhooks/{id}/.... #12 planned /webhooks/* for management and sneak's own words on #3 were "URLs under /webhooks/"; "hook" is a third word for the entity, and /hook/{id} beside /h/{uuid} invites confusing a webhook's ID with an entrypoint's secret. /h/{uuid} stays.
  • delivery.Guard, NewGuard: SSRFGuard, NewSSRFGuard. The README's term is the SSRF guard; Guard alone says nothing.
  • EventLogView: EventView. It is also the event page's view.
  • The README heading "Entrypoints" (the section about script/): "Development scripts". It collides with the Entrypoint entity.

Disclosures: nothing was built or run; the review touched no file in the repo; the parent-table claim rests on reading gorm.io/gorm@v1.25.5/migrator/migrator.go as well as the tree; the totals item is a product question, stated as one.

Model: fable-5-1

Reviewed `next` at `fe5e0d41733617022fb3a7b003c3c2b118059111` (2026-10-03, "Install ESLint and prettier with yarn 4 through corepack"). Read in full: every non-test Go file in `internal/database`, in `internal/delivery` except `ssrf.go`, in `internal/handlers` except `auth.go`, `profile.go`, `settings.go`, `index.go`, `healthcheck.go` and `metrics.go`, in `internal/server` and `cmd/webhooker`; `README.md` and `TODO.md`; the templates for the words they show; and the tracker history behind the names (https://git.eeqj.de/sneak/webhooker/issues/3, https://git.eeqj.de/sneak/webhooker/issues/12, https://git.eeqj.de/sneak/webhooker/pulls/16, https://git.eeqj.de/sneak/webhooker/issues/85, https://git.eeqj.de/sneak/webhooker/issues/206, https://git.eeqj.de/sneak/webhooker/issues/274, https://git.eeqj.de/sneak/webhooker/issues/316, https://git.eeqj.de/sneak/webhooker/issues/399, https://git.eeqj.de/sneak/webhooker/issues/477). Not covered, because they are the machinery around the model rather than the model: `config`, `middleware`, `session`, `metrics`, `logger`, `gormlog`, `logfield`, `lifecycle`, `datadir`, `resetpw`, `reqtls`, `banner`, the SSRF guard's internals, the scripts, the JavaScript and the tests. Nothing was built or run and no file in the repo was touched; every claim below was checked by reading the tree at that commit, and confidence is high for every item unless the item says otherwise. Items, most valuable first. Each gives the change, where it applies, why it makes the model clearer or simpler, its rough size, and what it depends on. - **Make a delivery task carry only identifiers, and read the event and target rows when the attempt runs.** - Where: `internal/delivery/engine.go` (`Task`, `buildEventFromTask`, `buildTargetFromTask`, `buildRecoveryTask`, `hydrateEvent`, `MaxInlineBodySize`), `internal/handlers/webhook.go` (`buildDeliveryTasks`, `inlineBody`), `internal/handlers/delivery_replay.go` (`createReplayDelivery`, `replayBody`), the README's "Request Flow" and "Dependency Injection" sections. - Why: the README still promises that a task carries everything "so the engine can deliver without reading from any database", but every task now reads the delivery row (`loadDelivery`) and the event row (`hydrateEvent`, for `created_at`), and a retry reads the target row as well (`abandonRetryForMissingTarget`). The copies buy nothing and cost a 14-field struct built in three places and turned back into model structs in two more. They also carry a wrong value: a retry chain keeps delivering with the target's settings as they were when the chain began (the comment on `abandonRetryForMissingTarget` says so), so an edit made during a backoff is ignored until a restart, while replay deliberately uses the current settings. With a task of delivery ID and webhook ID (the attempt number is the count of recorded attempts plus one, which recovery already computes), every attempt reads the current target, no body sits in a 10,000-deep channel, and the three builders become one line each. - Size: medium. Depends on nothing; it shrinks the next two items. - **Drop the relation fields that point across the two database files.** - Where: `internal/database/model_event.go` (`Event.Webhook`, `Event.Entrypoint`), `model_delivery.go` (`Delivery.Target`, and `Delivery.Event`, which is only ever filled in memory), `model_target.go` (`Target.Deliveries`), `event_db_isolation.go` (`omitAssociations`, `purgeTargetRows`, `eventDBSweptVersion`), `webhook_db_manager.go` (`openDB`), the `d.Event` and `d.Target` reads in `internal/delivery/target_*.go`, and `Omit(clause.Associations)` in `internal/handlers/delivery_replay.go`. - Why: the configuration tier and the event tier live in different files, so these relations can never be loaded with `Preload`, yet GORM treats them as real. `AutoMigrate` adds the parent tables to every `events-*.db`: `targets`, confirmed by the sweep that exists for it and by `TestEventDBHoldsNoTargetRows`, and `webhooks`, `entrypoints` and `users` by the same rule (verified in GORM's `migrator.go`, `ReorderModels` with `autoAdd`). It declares foreign keys to those empty tables, inert only because `SQLiteDSN` never turns `foreign_keys` on; turned on, every delivery insert would fail. And the association save copied target rows, credentials included, into those files (https://git.eeqj.de/sneak/webhooker/issues/206). The guard callback, the purge-and-VACUUM sweep with its `user_version` stamp, and the `Omit` at the replay site all exist to fight relations nothing uses; the purge is also upgrade-path code for files written by an earlier build, which the project has ruled out. Targets then receive the event and the target as plain arguments to `Deliver`, which is all they read from `d.Event` and `d.Target` today. - Size: medium. Depends on the task item above, or can precede it by passing the two values separately. - **Remove soft delete from the event tier.** - Where: `internal/database/base_model.go` (one base with `DeletedAt` for the configuration tier, one without for events, deliveries and attempts), the repeated `DeletedAt` fields and index tags in `model_event.go`, `model_delivery.go` and `model_delivery_result.go`, the `Unscoped()` calls in `retention.go`, the raw `deleted_at IS NULL` in `internal/handlers/event_body.go` and `event_log.go`, the README's "Event-tier indexes" and "Common Fields" sections. - Why: nothing ever soft-deletes an event, a delivery or an attempt; the only deletes in the event tier are retention's hard deletes. Yet every event-tier query carries `deleted_at IS NULL`, six of the seven event-tier indexes include `deleted_at` only to keep SQLite off the `deleted_at` index, the models repeat the field to put it in those indexes, and the README needs a paragraph on column order. Without the column the indexes are the plain ones a reader expects (`status, finished_at, target_id`; `event_id`; `delivery_id`; `created_at`; `resubmitted_from_id`; `entrypoint_id, resubmitted_from_id, created_at`) and the `Unscoped` calls and raw predicates go. `UpdatedAt` is likewise never read on events or attempts and can go with it; deliveries keep it for the pending sweep. - Size: small to medium. Depends on nothing. - **Store a target's settings as columns instead of a per-type JSON string.** - Where: `internal/database/model_target.go` (`Config`), `internal/delivery/target_http.go` (`HTTPTargetConfig`, `parseHTTPConfig`), `target_slack.go` (`SlackTargetConfig`, `parseSlackConfig`), `target_database_archive.go` (`databaseTargetConfig`, `parseArchiveExpiry`), `target_database_rotation.go` (`parseArchiveRotation`), `target_config_view.go`, `target_config_edit.go`, `target_redact.go` (`targetSecrets`), `internal/handlers/target_create.go` (`buildTargetConfig`, its three builders, `marshalTargetConfig`), `target_edit.go` (`configUnreadableMessage`), the README's "Target" section. - Why: a target has six settings in all (destination URL, extra headers, timeout, attempts, archive expiry, archive rotation). Keeping four of them in a JSON string whose shape depends on `Type` means every reader parses first and every reader carries a "configuration could not be read" branch (the view, the edit form, the delivery, the archive sweep, the redactor), the forms encode, the two URL-bearing types spell the same thing differently (`url`, `webhookUrl`), the credential lives in a string that must be tagged `json:"-"` and masked as a unit, and `MaxRetries` is the one setting that lives outside the string. As columns (`URL`, `Headers`, `TimeoutSeconds`, `MaxAttempts`, `ArchiveExpiry`, `ArchiveRotation`; the ones a type does not use stay empty) the struct says what a target can have, validation happens once at the form, masking is per field, the view types can live with the handlers that render them, and encryption at rest (https://git.eeqj.de/sneak/webhooker/issues/212), if it ever comes, covers two named columns instead of a blob. - Size: large. Depends on nothing; the attempts item below folds into it. - **One recovery routine, run at start and on every tick.** - Where: `internal/delivery/engine.go`: `recoverInFlight`, `recoverWebhookDeliveries`, `recoverRetryingDeliveries`, `recoverSingleRetry` and `recoverPendingDeliveries` on one side; `retrySweep`, `sweepOrphanedRetries`, `sweepWebhookRetries`, `sweepSingleRetry` and `sweepWebhookPending` on the other; `target.go` (`rescheduler`: `remainingBackoff`, `backoffElapsed`) and their implementations in `target_http.go`. - Why: the start-up pass and the 60-second sweep do the same job (list the webhooks, list each one's `retrying` and `pending` deliveries, settle those that already have a successful attempt, hand the rest to `takeForRedispatch`) in two parallel sets of functions. The only differences are the 15-minute age bound on pending rows and whether a retrying row goes to a timer for `remainingBackoff` or straight to the channel once `backoffElapsed`; a timer with a zero delay is "now", so one routine with a minimum-age parameter (zero at start) covers both and `backoffElapsed` goes. A reader then learns one recovery path. This keeps recovery engine-owned, which is also the recommendation recorded on https://git.eeqj.de/sneak/webhooker/issues/85. - Size: medium. Depends on nothing; smaller after the task item. - **Move "store the event, create its deliveries, queue them" from the handlers into the delivery engine.** - Where: `internal/handlers/webhook.go` (`eventSource`, `createAndFanOut`, `buildDeliveryTasks`), `event_resubmit.go` (`queueResubmit`), `delivery_replay.go` (`createReplayDelivery`), `internal/delivery/engine.go` (`Notifier`, `Archives`, `CircuitBreakers`) and the three bridge functions in `cmd/webhooker/main.go` that provide them. - Why: the write path that defines the product (one event row, one pending delivery per active target and the totals, in one transaction, then queue) lives in the HTTP package and is spelled twice (fan-out in `createAndFanOut`, one delivery in `createReplayDelivery`), while the engine, which owns deliveries everywhere else, learns of them through a `Notifier` carrying copies. With two methods on the engine (store an event and queue its deliveries; queue one more delivery of a stored event), the receiver, resubmit and replay handlers become request parsing plus one call, the handlers depend on one `*delivery.Engine` instead of three interfaces and three bridges, and the totals bookkeeping has one home. - Size: medium. Depends on the task item. - **Give the archive files their own package.** - Where: `internal/delivery/target_database.go`, `target_database_archive.go`, `target_database_rotation.go`, `target_database_export.go` and `archive_sweeper.go` (about 2,000 of the package's 7,500 non-test lines), `Handlers.renameMu` and the `Archives` interface, `internal/handlers/target_download.go` and `target_list.go` (`archiveFileView`). - Why: the delivery package is "deliver an event to a target", but a quarter of it is file management for one target type (naming, rotation, close and reopen, pruning, rename with rollback, export, the idle sweep), and the lock that orders renames against writes and exports lives in the HTTP handlers struct and is handed into `delivery.NewArchiveExport`. In a package of its own (`internal/archive`, say) the writers, the sweeper, the export and the one lock live together, the archive target type becomes a few lines that call it, and the handlers call it for rename, download and the row's file figures. - Size: medium, mostly moving code. Depends on nothing; pairs with the `database` to `archive` rename below. - **Store retention as a plain day count, 0 meaning forever.** - Where: `internal/database/model_webhook.go` (`RetentionForeverDays`, `BeforeSave`, `retainsForever`, the `gorm:"default:30"` tag), `retention.go` (`retentionCutoff`), `internal/handlers/shared.go` (`parseRetentionDays`), the README's "Webhook" section. - Why: the 365000-day sentinel exists only because the column default of 30 turns a stored 0 back into 30. Apply the default in the create form (it already falls back to `DefaultRetentionDays`) and drop the column default, and a 0 is stored as 0: no sentinel, no `BeforeSave` rewrite, no three bands of accepted values to document, and the edit form shows 0 for forever, which is what its copy already says. `MaxFiniteRetentionDays` stays. - Size: small. Depends on nothing. - **Count delivery attempts rather than retries, with a minimum of one.** - Where: `internal/database/model_target.go` (`MaxRetries`), `internal/handlers/target_retries.go`, `internal/delivery/target_http.go` (`httpCore.deliver`, `handleRetry`), `target_config_view.go` (`maxRetriesField`), the README's "Target" section. - Why: today 0 and 1 both mean one attempt (0 without the circuit breaker, 1 with it), the UI shows both as "1" and explains one of them in words, and the README spends two paragraphs on the difference; the name is the confusion https://git.eeqj.de/sneak/webhooker/issues/316 and https://git.eeqj.de/sneak/webhooker/issues/399 recorded. Store the number of attempts, at least 1: one attempt is fire-and-forget, more than one retries with the breaker, and the form's default is 1. - Size: small. Depends on nothing; folds into the columns item if that is done. - **Delete the `APIKey` model and the empty `/api/v1` route group.** - Where: `internal/database/model_apikey.go`, `model_user.go` (`User.APIKeys`), `models.go`, `internal/server/routes.go` (`/api/v1`), the README's "APIKey", "API (Planned)" and "Authentication" entries. - Why: nothing reads or writes the model (its only references are the migration list and the `User` relation), so it is a table, a relation and a documented entity for a feature that does not exist. TODO.md lists the API as a future step, and the model can return with it. - Size: small. Depends on nothing. - **Decide whether lifetime figures must outlive retention; if not, drop the three totals tables.** - Where: `internal/database/model_totals.go`; the `AddEventTotals`, `AddTargetTotals` and `AddEntrypointTotals` calls in `internal/handlers/webhook.go`, `delivery_replay.go`, `internal/delivery/engine.go` (`writeDeliveryStatus`) and `internal/database/retention.go` (`deleteEvents`); the readers in `internal/handlers/webhook_stats.go`, `webhook_list.go`, `target_list.go` and `entrypoint_view.go`. - Why: three counter tables, kept in step from four files, exist so the statistics pane can show counts from before retention deleted the rows, and last-event times without a scan. If the figures within retention are enough (the pane shows both today), the tables and their bookkeeping go and every figure is a count over an index. What is given up: the lifetime counts, and an entrypoint's last-event time once retention has removed that event. This is a product question for sneak, not a defect; confidence here is about the cost, not about the answer. - Size: medium. Depends on nothing. Names. Each line gives the current name, the proposed name and the reason. A proposed word that the repo's documents do not already use is marked "new word" for sneak to approve. - `HandleWebhook`, `handlers/webhook.go`, `processWebhookRequest`, `readWebhookBody`, `finishWebhookResponse`, `maxWebhookBodySize`, `setupWebhookRoutes`, and the log lines "webhook request received" and "webhook event created": `HandleReceive`, `handlers/receiver.go`, `processReceivedRequest`, `readEventBody`, `finishReceiverResponse`, `maxEventBodySize`, `setupReceiverRoutes`, "event received", "event stored". The README calls `/h/{uuid}` the receiver; "webhook" then means only the configured thing, as https://git.eeqj.de/sneak/webhooker/issues/12 decided. - `WebhookDBManager` (and `dbMgr`, `WebhookDBMgr`, `DBManager`): `EventDBManager`. The README calls the files per-webhook event databases and the code already says "event database" in its errors; "webhook DB" reads as the webhooks table. - `database.Database`: `MainDB`. The README and `MainDBFileName` call it the main database; `database.Database` names nothing. - `Entrypoint.Path`: `Entrypoint.Secret` (new word as a field name). It stores a bare UUID, not a path (the README: "The `/h/` prefix is route only and is not stored"), and the README calls the URL the authentication secret. - `TargetTypeDatabase` and the stored value `database`, `databaseTarget`, `databaseTargetConfig`, `buildDatabaseTargetConfig`, `target_database*.go`: `TargetTypeArchive` and `archive`, `archiveTarget`, `archiveConfig`, `buildArchiveConfig`, `target_archive*.go`. The UI already calls the type Archive (https://git.eeqj.de/sneak/webhooker/issues/399), and "database" otherwise means the main database, an event database or an archive file. - `Target.MaxRetries`, the form field `max_retries`, `parseMaxRetries`, `maxTargetRetries`: `MaxAttempts`, `max_attempts`, `parseMaxAttempts`, `maxTargetAttempts`. It holds attempts in all, as the UI label "Delivery attempts" says. - `DeliveryResult`, the table `delivery_results`, `AttemptNum`, `Duration`: `DeliveryAttempt`, `delivery_attempts`, `Number`, `DurationMS`. The UI, the README and the handlers call these attempts (`delivery_attempts.html`, "Attempt 3", `loadDeliveryResults` returns attempts), and the view already says `DurationMS`. - `SlackTargetConfig.WebhookURL`, the key `webhookUrl`, `errMissingWebhookURL`: `URL`, `url`, `errMissingTargetURL` (shared with the http type). The form field is `url` for both types and `TargetConfigForm` maps both to `URL`; one meaning of "webhook" fewer. Moot if the columns item lands. - `HandleSourceList`, `HandleSourceCreate`, `HandleSourceCreateSubmit`, `HandleSourceDetail`, `HandleSourceEdit`, `HandleSourceEditSubmit`, `HandleSourceDelete`, `renderSourceDetail`, `setupSourceRoutes`, the route parameter `sourceID`, `sourceFormInput`, `newSourceFormData`; the templates `sources_list.html`, `sources_new.html`, `source_detail.html`, `source_edit.html`; the tests `source_*_test.go`: the same names with `Webhook` or `webhook` (`HandleWebhookList`, `webhookID`, `webhook_list.html` and so on). The entity was renamed in https://git.eeqj.de/sneak/webhooker/issues/12; these kept the old name. `HandleSourceLogs` and `source_logs.html` are the event log: `HandleEventLog`, `event_log.html`. - The routes `/hooks`, `/hooks/new`, `/hook/{id}/...`: `/webhooks`, `/webhooks/new`, `/webhooks/{id}/...`. https://git.eeqj.de/sneak/webhooker/issues/12 planned `/webhooks/*` for management and sneak's own words on https://git.eeqj.de/sneak/webhooker/issues/3 were "URLs under `/webhooks/`"; "hook" is a third word for the entity, and `/hook/{id}` beside `/h/{uuid}` invites confusing a webhook's ID with an entrypoint's secret. `/h/{uuid}` stays. - `delivery.Guard`, `NewGuard`: `SSRFGuard`, `NewSSRFGuard`. The README's term is the SSRF guard; `Guard` alone says nothing. - `EventLogView`: `EventView`. It is also the event page's view. - The README heading "Entrypoints" (the section about `script/`): "Development scripts". It collides with the Entrypoint entity. Disclosures: nothing was built or run; the review touched no file in the repo; the parent-table claim rests on reading `gorm.io/gorm@v1.25.5/migrator/migrator.go` as well as the tree; the totals item is a product question, stated as one. Model: fable-5-1
sneak was assigned by clawbot 2026-10-03 14:31:04 +02:00
Sign in to join this conversation.
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#499