diff --git a/README.md b/README.md index ca836d8..b3ec850 100644 --- a/README.md +++ b/README.md @@ -2112,15 +2112,15 @@ writes go on meanwhile, since under WAL a reader never blocks a writer. Until the open file is closed its `-wal` cannot be checkpointed past what the download reads, so a long download lets that `-wal` grow. -Deleting a webhook releases its archives: the delivery engine's cached archive -writers are dropped and their file handles closed, so nothing lingers after the -webhook is gone. The archive **files themselves are deliberately left on disk**. -Unlike the event database — per-webhook working storage that is hard-deleted -with the webhook — an archive is long-term storage an operator may still want to -keep or move away for offline retention, and destroying it as a side effect of -deleting a webhook would be unrecoverable. Removing an `archive-….db` is the -operator's call. Deleting a `database` target releases its writer the same way, -and for the same reason leaves its file alone. +Deleting a webhook releases its archives: its cached archive writers are dropped +and their file handles closed, so nothing lingers after the webhook is gone. The +archive **files themselves are deliberately left on disk**. Unlike the event +database — per-webhook working storage that is hard-deleted with the webhook — +an archive is long-term storage an operator may still want to keep or move away +for offline retention, and destroying it as a side effect of deleting a webhook +would be unrecoverable. Removing an `archive-….db` is the operator's call. +Deleting a `database` target releases its writer the same way, and for the same +reason leaves its file alone. The **Slack target type** sends webhook events as formatted messages to any Slack-compatible incoming webhook URL (works with Slack, Mattermost, and other @@ -3009,6 +3009,12 @@ webhooker/ ├── cmd/webhooker/ │ └── main.go # Entry point: subcommand dispatch; no args locks DATA_DIR and wires fx ├── internal/ +│ ├── archive/ +│ │ ├── archive.go # The archive writer of each database target, eviction, rename, and the rename lock +│ │ ├── writer.go # Archive file lifecycle and pruning +│ │ ├── rotation.go # Archive rotation and file names +│ │ ├── download.go # Archive download as gzipped JSON +│ │ └── sweeper.go # Periodic pruning of idle archives │ ├── banner/ │ │ └── banner.go # Ruled block for the one credential shown in the clear │ ├── resetpw/ @@ -3052,13 +3058,9 @@ webhooker/ │ │ ├── target.go # Target interface, Task, Scheduler │ │ ├── target_http.go # HTTP target (retries, circuit breaker) │ │ ├── target_slack.go # Slack/Mattermost incoming-webhook target -│ │ ├── target_database.go # Database archive target -│ │ ├── target_database_archive.go # Archive file lifecycle and pruning -│ │ ├── target_database_rotation.go # Archive rotation and file names -│ │ ├── target_database_export.go # Archive download as gzipped JSON +│ │ ├── target_database.go # Database archive target, writing through internal/archive │ │ ├── target_log.go # Log target (stdout) │ │ ├── target_config_view.go # Masked target config for templates -│ │ ├── archive_sweeper.go # Periodic pruning of idle archives │ │ ├── queue_depth.go # Periodic sampler behind the queue-depth gauges │ │ ├── url_mask.go # Strips credentials from *url.Error │ │ └── ssrf.go # SSRF prevention (IP validation, safe HTTP transport) @@ -3149,14 +3151,16 @@ Components are wired via Uber fx in this order: 11. `metrics.New` — The delivery collectors, registered on that registry 12. `middleware.New` — HTTP middleware 13. `delivery.New` — Event-driven delivery engine -14. `delivery.NewArchiveSweeper` — Periodic pruning of idle archives -15. `delivery.Engine` → `delivery.Notifier` — interface bridge -16. `delivery.Engine` → `delivery.Archives` — interface bridge so deleting or +14. `archive.NewWriters` — The database targets' archive writers, and the lock + every rename of their files runs under +15. `archive.NewSweeper` — Periodic pruning of idle archives +16. `delivery.Engine` → `delivery.Notifier` — interface bridge +17. `archive.Writers` → `archive.Archives` — interface bridge so deleting or renaming a webhook or target reaches its archive files -17. `server.New` — HTTP server and router +18. `server.New` — HTTP server and router The server starts via -`fx.Invoke(func(*server.Server, *delivery.Engine, *database.RetentionReaper, *delivery.ArchiveSweeper) {})`, +`fx.Invoke(func(*server.Server, *delivery.Engine, *database.RetentionReaper, *archive.Sweeper) {})`, which triggers the fx lifecycle hooks in dependency order. The `delivery.Notifier` interface allows the webhook handler to send self-contained `delivery.Task` slices to the engine without a direct package dependency. Each @@ -3325,7 +3329,7 @@ order under a **5 second budget** (`fx.StopTimeout` in `cmd/webhooker/main.go`). That budget covers the whole sequence, not each hook. The order, read off the fx stop-hook log: -1. `ArchiveSweeper` +1. `archive.Sweeper` 2. `RetentionReaper` 3. `server` — the HTTP drain, bounded by `server.ShutdownTimeout` (**3 seconds**) and by what the hooks before it left, then a Sentry flush if @@ -3368,7 +3372,7 @@ skipped when that leaves too little to be worth attempting — so a full-length drain means Sentry events are dropped rather than the database close being skipped. -This does not make the database close unconditional. A slow `ArchiveSweeper` or +This does not make the database close unconditional. A slow `archive.Sweeper` or `RetentionReaper` is enough to cut the shutdown short, not only one that consumes the whole budget: what they spend comes out of the drain first, so after 2 seconds of theirs a request still in flight gets 1 second to finish, and diff --git a/cmd/webhooker/main.go b/cmd/webhooker/main.go index 5990801..7ad426c 100644 --- a/cmd/webhooker/main.go +++ b/cmd/webhooker/main.go @@ -10,6 +10,7 @@ import ( "go.uber.org/fx" "go.uber.org/fx/fxevent" + "sneak.berlin/go/webhooker/internal/archive" "sneak.berlin/go/webhooker/internal/config" "sneak.berlin/go/webhooker/internal/database" "sneak.berlin/go/webhooker/internal/datadir" @@ -49,7 +50,7 @@ import ( // before the server may already have spent. // // This does not make the database close unconditional: the -// ArchiveSweeper and RetentionReaper hooks run before the server. +// archive.Sweeper and RetentionReaper hooks run before the server. // What they spend comes out of the drain first, but past 3s it comes // out of the reserve, and they can consume the whole budget. const stopTimeout = 5 * time.Second @@ -208,14 +209,15 @@ func newApp() *fx.App { // disagree about a destination. delivery.NewGuard, delivery.New, - delivery.NewArchiveSweeper, + archive.NewWriters, + archive.NewSweeper, // Wire *delivery.Engine as delivery.Notifier so the // webhook handler can notify the engine of new deliveries. func(e *delivery.Engine) delivery.Notifier { return e }, - // Wire *delivery.Engine as delivery.Archives so deleting + // Wire *archive.Writers as archive.Archives so deleting // or renaming a webhook or target reaches its archive // files. - func(e *delivery.Engine) delivery.Archives { return e }, + func(w *archive.Writers) archive.Archives { return w }, // Wire *delivery.Engine as delivery.CircuitBreakers so // the pages can show a target whose deliveries are // paused. @@ -227,7 +229,7 @@ func newApp() *fx.App { *server.Server, *delivery.Engine, *database.RetentionReaper, - *delivery.ArchiveSweeper, + *archive.Sweeper, ) { }, ), diff --git a/cmd/webhooker/main_test.go b/cmd/webhooker/main_test.go index 160cb1d..dbc6836 100644 --- a/cmd/webhooker/main_test.go +++ b/cmd/webhooker/main_test.go @@ -252,7 +252,7 @@ const tailHeadroom = 2 * time.Second // can produce, since a shorter drain leaves the flush more room and // the worst case is not necessarily at either extreme. // -// Nor does the hook start on a full budget: the ArchiveSweeper and +// Nor does the hook start on a full budget: the archive.Sweeper and // RetentionReaper hooks run before it, and whatever they spent is // gone. The outer sweep walks every amount they can spend. Once they // have eaten into the headroom themselves, the hook must spend diff --git a/internal/archive/archive.go b/internal/archive/archive.go new file mode 100644 index 0000000..98c8e34 --- /dev/null +++ b/internal/archive/archive.go @@ -0,0 +1,458 @@ +// Package archive manages the SQLite archive files the database target +// writes: how they are named and rotated, the writers that open, write, +// close and reopen them, pruning the rows past a target's expiry, +// renaming a target's files and moving them back when that fails, the +// gzipped JSON export, the idle sweep, and the locks that keep a rename +// apart from archive writes, from other edits and from exports. +package archive + +import ( + "fmt" + "log/slog" + "strings" + "sync" + "time" + + "go.uber.org/fx" + "sneak.berlin/go/webhooker/internal/database" +) + +// archiveNameMaxLen is how many characters of a webhook or target +// name an archive file name keeps. +const archiveNameMaxLen = 40 + +// Archives is how the handlers keep the database targets' archive +// files in step with the configuration. Deleting a webhook or a +// target releases the cached archive writers, whose open file +// handles would otherwise outlive them; renaming one renames the +// archive files, which are named for the webhook and the target +// (see FileName). Lock and Unlock take and release the lock every +// rename runs under (see Writers.Lock). +// +// It is an interface so that the handlers' tests can stand in for +// Writers. +// +// Neither eviction deletes an archive file. Both are idempotent +// and are no-ops for a webhook or target with no writer. +type Archives interface { + sync.Locker + EvictWebhook(webhookID string) + EvictTarget(targetID string) + Rename(targetID, webhookName, targetName string) error +} + +// WritersParams holds the fx dependencies for Writers. +type WritersParams struct { + fx.In + + Database *database.Database + DBManager *database.WebhookDBManager + Logger *slog.Logger +} + +// Writers holds the archive writer of each database target, and the +// lock every rename of archive files runs under. The database target +// writes an event through it, the handlers rename and evict through +// it, the archive sweeper prunes through it, and the delivery engine +// evicts every writer when it stops. See archiveWriter for the +// close/reopen, auto-recreate, and expiry semantics. +type Writers struct { + db *database.Database + dbMgr *database.WebhookDBManager + log *slog.Logger + + // writers holds one archive writer per database target, keyed + // by target ID. + mu sync.Mutex + writers map[string]*archiveWriter + + // renameMu makes the webhook edit, the target edit and target + // creation run one at a time, each held from loading the stored + // names through the archive rename, the save and any move back. + // Interleaved, one could rename an archive between another's + // rename and save, leaving the file named for one edit and the + // stored names from the other. An archive download holds it while + // it reads the stored names and lists the files they give, and + // again for each file while it finds the file under the names + // stored then and opens it. Lock and Unlock take and release it. + renameMu sync.Mutex +} + +// NewWriters builds the archive writers. It opens no archive file: +// each target's writer is made on its first use. +func NewWriters(params WritersParams) *Writers { + return &Writers{ + db: params.Database, + dbMgr: params.DBManager, + log: params.Logger, + } +} + +// Lock takes the lock every rename of archive files runs under: see +// renameMu for who holds it and for how long. +func (ws *Writers) Lock() { + ws.renameMu.Lock() +} + +// Unlock releases the lock Lock takes. +func (ws *Writers) Unlock() { + ws.renameMu.Unlock() +} + +// FileName returns the file name of a database target's +// archive: archive-WEBHOOKNAME-TARGETNAME-TARGETID.db, with both +// names passed through archiveNamePart. The target ID keeps the +// name unique when two targets' names come out the same. +func FileName(webhookName, targetName, targetID string) string { + return "archive-" + archiveNamePart(webhookName) + "-" + + archiveNamePart(targetName) + "-" + targetID + ".db" +} + +// archiveNamePart makes a webhook or target name safe to put in a +// file name. It is lowercased; ASCII letters and digits are kept, +// every other run of characters becomes a single "-", and no "-" is +// left at either end. It is cut to archiveNameMaxLen characters, and +// a name with nothing left is "unnamed". +func archiveNamePart(name string) string { + var b strings.Builder + + dash := false + + for _, r := range strings.ToLower(name) { + if (r < 'a' || r > 'z') && (r < '0' || r > '9') { + dash = b.Len() > 0 + + continue + } + + if dash { + b.WriteByte('-') + + dash = false + } + + b.WriteRune(r) + } + + part := b.String() + if len(part) > archiveNameMaxLen { + part = strings.TrimRight(part[:archiveNameMaxLen], "-") + } + + if part == "" { + return "unnamed" + } + + return part +} + +// Write writes the full event of a delivery to a database target as +// a row into the target's archive database, honouring the optional +// per-target expiry and rotation parsed from the target config JSON. +// With rotation, the event goes to the file for the period of its +// receive time. +func (ws *Writers) Write(d *database.Delivery) error { + webhookID := d.Event.WebhookID + if webhookID == "" { + return errArchiveMissingWebhookID + } + + expiry, err := ParseExpiry(d.Target.Config) + if err != nil { + return err + } + + rotation, err := ParseRotation(d.Target.Config) + if err != nil { + return err + } + + // An event whose stored row was gone before its delivery ran has + // no receive time (see delivery's Engine.hydrateEvent), and goes + // to the file for now. + receivedAt := d.Event.CreatedAt + if receivedAt.IsZero() { + receivedAt = time.Now() + } + + w, err := ws.writerFor(d.TargetID) + if err != nil { + return err + } + + row := archivedEvent{ + EventID: d.Event.ID, + WebhookID: webhookID, + EntrypointID: d.Event.EntrypointID, + Method: d.Event.Method, + RawQuery: d.Event.RawQuery, + Headers: d.Event.Headers, + Body: d.Event.Body, + ContentType: d.Event.ContentType, + } + + return w.write(row, expiry, archivePeriod(rotation, receivedAt)) +} + +// Rename implements Archives. It renames every one of a database +// target's archive files to FileName(webhookName, targetName, +// targetID), each keeping the period in its name. It goes through the +// target's writer, so the move holds the lock that the target's +// archive writes and the idle sweep take, and later writes use the +// new name. It never replaces a file: if one already has a new name, +// the error is ErrNameTaken. +// +// The writer is created if there is none, and it stays cached. The +// handlers rename before they save the new name, so until the save +// the main database still has the old one; a delivery in that window +// must find this writer rather than build one from the old name. +func (ws *Writers) Rename( + targetID, webhookName, targetName string, +) error { + w, err := ws.writerFor(targetID) + if err != nil { + return err + } + + return w.rename(FileName(webhookName, targetName, targetID)) +} + +// EvictTarget implements Archives. It drops a database target's +// archive writer from the registry and closes its handle, so a +// deleted target does not leave a writer (and an open archive handle +// within its debounce window) alive for the process lifetime. +// +// The map entry is removed under the registry lock, which is +// then released before the handle is closed under the writer's +// own lock: that ordering keeps the registry available to other +// targets while an in-flight write on this one drains, and +// closing under the writer's lock means eviction can never race +// a write. +// +// Eviction is idempotent and silent for a target with no writer, +// which is the common case: only a database target that has +// received an event or been renamed has one. It never deletes the +// archive file. +func (ws *Writers) EvictTarget(targetID string) { + ws.mu.Lock() + + w, ok := ws.writers[targetID] + if ok { + delete(ws.writers, targetID) + } + + ws.mu.Unlock() + + if !ok { + return + } + + w.evict() + + ws.log.Info( + "evicted archive writer", + "target_id", targetID, + "path", w.path, + ) +} + +// EvictWebhook implements Archives. It evicts, exactly as EvictTarget +// does, the writer of every database target of a webhook. The archive +// files themselves are left on disk — they are long-term storage the +// operator owns. +func (ws *Writers) EvictWebhook(webhookID string) { + ws.mu.Lock() + + var gone []*archiveWriter + + for targetID, w := range ws.writers { + if w.webhookID == webhookID { + delete(ws.writers, targetID) + + gone = append(gone, w) + } + } + + ws.mu.Unlock() + + for _, w := range gone { + w.evict() + + ws.log.Info( + "evicted archive writer", + "webhook_id", webhookID, + "path", w.path, + ) + } +} + +// EvictAll evicts every cached archive writer, exactly as EvictTarget +// does for one target. The delivery engine calls it at shutdown, once +// its workers have returned. Closing the last handle on an archive +// moves the contents of its -wal into the .db and removes the +// -wal, so a clean stop leaves each archive as a single file. +func (ws *Writers) EvictAll() { + ws.mu.Lock() + + writers := ws.writers + ws.writers = nil + + ws.mu.Unlock() + + for _, w := range writers { + w.evict() + } +} + +// writerFor returns the archive writer for a database target, +// creating and caching it on first use. Each target has one writer +// so its close/reopen debounce state is shared across concurrent +// deliveries, and so a rename and the idle sweep take the same lock +// as its writes. +func (ws *Writers) writerFor( + targetID string, +) (*archiveWriter, error) { + ws.mu.Lock() + defer ws.mu.Unlock() + + w, ok := ws.writers[targetID] + if !ok { + var err error + + w, err = ws.newWriter(targetID) + if err != nil { + return nil, err + } + + if ws.writers == nil { + ws.writers = make(map[string]*archiveWriter) + } + + ws.writers[targetID] = w + } + + // A delivery claims the entry: even if the idle sweep created + // it moments ago, it now belongs to the registry proper and + // the sweep must leave it in place when it finishes. + w.sweepOwned = false + + return w, nil +} + +// sweepWriterFor returns the archive writer the idle sweep should +// prune a target's archive through, together with whether the sweep +// itself created the registry entry. +// +// The sweep must route its prune through the registered writer so +// the writer's mutex orders it against concurrent writes, but it +// must never leave a registry entry behind: a sweep that ran +// concurrently with the target's deletion would otherwise +// re-create an entry that nothing will ever evict again, which is +// exactly the leak eviction exists to prevent. An entry the sweep +// creates is therefore marked sweep-owned and handed back to +// releaseSweepWriter when the sweep is done. +func (ws *Writers) sweepWriterFor( + targetID string, +) (*archiveWriter, bool, error) { + ws.mu.Lock() + defer ws.mu.Unlock() + + w, ok := ws.writers[targetID] + if ok { + return w, false, nil + } + + w, err := ws.newWriter(targetID) + if err != nil { + return nil, false, err + } + + if ws.writers == nil { + ws.writers = make(map[string]*archiveWriter) + } + + w.sweepOwned = true + ws.writers[targetID] = w + + return w, true, nil +} + +// releaseSweepWriter drops a registry entry that the idle sweep +// created, so a sweep leaves the registry exactly as it found it. +// +// The entry is removed only if it is still the very writer the +// sweep installed and no delivery has claimed it in the meantime +// (writerFor clears sweepOwned when it hands a writer to the +// write path). Both conditions are evaluated under the registry +// lock, so an eviction that raced the sweep — which removes the +// entry outright — simply finds nothing left to do here, and a +// delivery that adopted the writer keeps a registered, evictable +// one. +func (ws *Writers) releaseSweepWriter( + targetID string, w *archiveWriter, +) { + ws.mu.Lock() + defer ws.mu.Unlock() + + cur, ok := ws.writers[targetID] + if !ok || cur != w || !cur.sweepOwned { + return + } + + delete(ws.writers, targetID) +} + +// newWriter builds the writer for a database target's archive. Its +// path is the one Path gives for the webhook and the target as +// the main database names them now; from then on only rename changes +// the name the writer uses. It does not touch the archive files. +func (ws *Writers) newWriter( + targetID string, +) (*archiveWriter, error) { + if ws.dbMgr == nil { + return nil, errArchiveNoDataDir + } + + var target database.Target + + err := ws.db.DB(). + Preload("Webhook"). + First(&target, "id = ?", targetID).Error + if err != nil { + return nil, fmt.Errorf( + "loading database target %s: %w", targetID, err, + ) + } + + w := newArchiveWriter( + Path(ws.dbMgr, &target.Webhook, &target), + ws.log, + ) + w.webhookID = target.WebhookID + + return w, nil +} + +// sweepArchive prunes one database target's archive of rows older +// than expiry, without requiring a write. A missing archive file is +// left missing (see sweepExpired), so a sweep never creates an +// archive for a target that has never received an event. +// +// It also never leaves a registry entry behind: an entry it had +// to create to reach the writer's mutex is released again once +// the prune is done, so a sweep racing a target deletion cannot +// resurrect the writer the eviction just dropped. +func (ws *Writers) sweepArchive( + targetID string, expiry time.Duration, +) error { + w, created, err := ws.sweepWriterFor(targetID) + if err != nil { + return err + } + + if created { + defer ws.releaseSweepWriter(targetID, w) + } + + return w.sweepExpired(expiry) +} diff --git a/internal/archive/archive_test.go b/internal/archive/archive_test.go new file mode 100644 index 0000000..e79df7f --- /dev/null +++ b/internal/archive/archive_test.go @@ -0,0 +1,713 @@ +package archive_test + +import ( + "database/sql" + "fmt" + "io/fs" + "log/slog" + "net/http" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gorm.io/driver/sqlite" + "gorm.io/gorm" + _ "modernc.org/sqlite" // Pure Go SQLite driver. + "sneak.berlin/go/webhooker/internal/archive" + "sneak.berlin/go/webhooker/internal/database" + "sneak.berlin/go/webhooker/internal/gormlog" +) + +const ( + // testContentType is the event content type used in tests. + testContentType = "application/json" + + // eventQuery is the query string an archived event carries. + eventQuery = "a=1&b=2" +) + +func archiveTestLogger() *slog.Logger { + return slog.New(slog.NewTextHandler( + os.Stderr, + &slog.HandlerOptions{Level: slog.LevelDebug}, + )) +} + +// openArchiveDBForRead opens an archive file read-only so a +// test can inspect the rows the writer persisted. +func openArchiveDBForRead( + t *testing.T, path string, +) *gorm.DB { + t.Helper() + + sqlDB, err := sql.Open( + "sqlite", + fmt.Sprintf("file:%s?mode=ro", path), + ) + require.NoError(t, err) + + t.Cleanup(func() { _ = sqlDB.Close() }) + + gdb, err := gorm.Open( + sqlite.Dialector{Conn: sqlDB}, + &gorm.Config{Logger: gormlog.New(slog.New(slog.DiscardHandler))}, + ) + require.NoError(t, err) + + return gdb +} + +// archiveFileSuffixes returns the archive file itself and the +// SQLite sidecars that accompany an open database. A test that +// asserts no archive was created has to check all of them. +func archiveFileSuffixes() []string { + return []string{"", "-wal", "-shm"} +} + +// removeArchiveFiles simulates an operator moving the archive +// away by deleting the SQLite file and its sidecar files. +func removeArchiveFiles(t *testing.T, path string) { + t.Helper() + + for _, suffix := range []string{ + "", "-wal", "-shm", "-journal", + } { + err := os.Remove(path + suffix) + if err != nil && !os.IsNotExist(err) { + t.Fatalf("removing %s%s: %v", path, suffix, err) + } + } +} + +// assertStoredTime asserts that text, a timestamp as SQLite holds it, +// is in the form GORM stores created_at in: the layout of +// time.Time.String, with no " m=+…" monotonic clock reading after it. +func assertStoredTime(t *testing.T, text string) { + t.Helper() + + _, err := time.Parse( + "2006-01-02 15:04:05.999999999 -0700 MST", text, + ) + assert.NoError(t, err, "stored as %q", text) +} + +// newEvent returns an event received now with the given body, as the +// webhook handler stores it. +func newEvent(body string) database.Event { + return database.Event{ + BaseModel: database.BaseModel{ + ID: uuid.New().String(), + CreatedAt: time.Now(), + }, + WebhookID: uuid.New().String(), + EntrypointID: uuid.New().String(), + Method: http.MethodPost, + Headers: `{"Content-Type":["application/json"]}`, + Body: body, + ContentType: testContentType, + } +} + +// databaseTargetDelivery returns a delivery of event to a database +// target, as the database target hands it to Writers.Write. +func databaseTargetDelivery( + event database.Event, tgt *database.Target, +) *database.Delivery { + return &database.Delivery{ + EventID: event.ID, + TargetID: tgt.ID, + Event: event, + Target: database.Target{ + Name: tgt.Name, + Type: database.TargetTypeDatabase, + Config: tgt.Config, + }, + } +} + +func TestArchiveWriter_WritesRow(t *testing.T) { + t.Parallel() + + path := filepath.Join(t.TempDir(), "archive-wh.db") + w := archive.NewExportArchiveWriter( + path, archiveTestLogger(), 0, + ) + + row := archive.ExportArchivedEvent{ + EventID: "ev-1", + WebhookID: "wh-1", + EntrypointID: "ep-1", + Method: "POST", + Headers: `{"X":"Y"}`, + Body: `{"hello":"world"}`, + ContentType: "application/json", + } + + require.NoError(t, w.Write(row, 0)) + assert.FileExists(t, path) + + var got []archive.ExportArchivedEvent + + require.NoError(t, w.DB().Find(&got).Error) + require.Len(t, got, 1) + assert.Equal(t, "ev-1", got[0].EventID) + assert.Equal(t, "wh-1", got[0].WebhookID) + assert.Equal(t, "ep-1", got[0].EntrypointID) + assert.Equal(t, row.Method, got[0].Method) + assert.Equal(t, row.ContentType, got[0].ContentType) + assert.JSONEq(t, `{"hello":"world"}`, got[0].Body) + assert.False(t, got[0].ArchivedAt.IsZero()) + + var archivedAt string + + require.NoError(t, w.DB().Raw( + "SELECT CAST(archived_at AS TEXT) FROM archived_events", + ).Row().Scan(&archivedAt)) + assertStoredTime(t, archivedAt) +} + +func TestArchiveWriter_RecreatesAfterRemoval( + t *testing.T, +) { + t.Parallel() + + path := filepath.Join(t.TempDir(), "archive-wh.db") + w := archive.NewExportArchiveWriter( + path, archiveTestLogger(), 0, + ) + + require.NoError(t, w.Write( + archive.ExportArchivedEvent{EventID: "a"}, 0, + )) + assert.FileExists(t, path) + + // The operator moves the archive away while the handle is + // still open. + removeArchiveFiles(t, path) + require.NoFileExists(t, path) + + // The next write recreates the file with a fresh schema and + // only the new row. + require.NoError(t, w.Write( + archive.ExportArchivedEvent{EventID: "b"}, 0, + )) + assert.FileExists(t, path) + + var got []archive.ExportArchivedEvent + + require.NoError(t, w.DB().Find(&got).Error) + require.Len(t, got, 1) + assert.Equal(t, "b", got[0].EventID) +} + +// TestArchiveHandleIsWAL closes the last gap in the durability +// evidence: the main and per-webhook tiers each assert their journal +// mode on a live handle, and the archive tier gets its settings from +// the same code path but nothing checked the running file. +func TestArchiveHandleIsWAL(t *testing.T) { + t.Parallel() + + w := archive.NewExportArchiveWriter( + filepath.Join(t.TempDir(), "archive-wal.db"), + archiveTestLogger(), 0, + ) + + require.NoError(t, w.Open(0)) + + var mode string + + row := w.DB().Raw("pragma journal_mode").Row() + require.NoError(t, row.Scan(&mode)) + assert.Equal(t, "wal", strings.ToLower(mode)) + + var busy string + + row = w.DB().Raw("pragma busy_timeout").Row() + require.NoError(t, row.Scan(&busy)) + assert.Equal(t, "10000", busy) +} + +// TestStatArchive proves Stat finds no file before the first +// write; after a write still held in the -wal, counts the -wal in the +// size and takes its later time as the last write; and finds no file +// again once the file has been moved away. +func TestStatArchive(t *testing.T) { + t.Parallel() + + path := filepath.Join(t.TempDir(), "archive-wh.db") + + _, err := archive.Stat(path) + require.ErrorIs(t, err, fs.ErrNotExist) + + // With the clock stopped, the reopen debounce never passes, so + // the handle stays open after the write. + stopped := time.Now() + w := archive.NewExportArchiveWriter(path, archiveTestLogger(), 0) + w.SetNow(func() time.Time { return stopped }) + + require.NoError(t, w.Write(archive.ExportArchivedEvent{EventID: "a"}, 0)) + + written := time.Date(2026, 1, 2, 3, 4, 5, 0, time.UTC) + earlier := written.Add(-time.Hour) + require.NoError(t, os.Chtimes(path, earlier, earlier)) + require.NoError(t, os.Chtimes(path+"-wal", written, written)) + + file, err := os.Stat(path) + require.NoError(t, err) + + wal, err := os.Stat(path + "-wal") + require.NoError(t, err) + require.Positive(t, wal.Size()) + + got, err := archive.Stat(path) + require.NoError(t, err) + assert.Equal(t, 1, got.Files) + assert.Equal(t, file.Size()+wal.Size(), got.Size) + assert.True(t, written.Equal(got.Written), got.Written) + + removeArchiveFiles(t, path) + + _, err = archive.Stat(path) + require.ErrorIs(t, err, fs.ErrNotExist) +} + +func TestArchiveWriter_ReopenDebounce(t *testing.T) { + t.Parallel() + + const debounce = 2 * time.Second + + path := filepath.Join(t.TempDir(), "archive-wh.db") + w := archive.NewExportArchiveWriter( + path, archiveTestLogger(), debounce, + ) + + // The writer measures its reopen debounce on this clock, which + // only the test moves, so how long the host takes between + // writes cannot change the result. + now := time.Now() + + w.SetNow(func() time.Time { return now }) + + require.NoError(t, w.Write( + archive.ExportArchivedEvent{EventID: "a"}, 0, + )) + require.NoError(t, w.Write( + archive.ExportArchivedEvent{EventID: "b"}, 0, + )) + + // Two writes inside the debounce window trigger only the + // initial open — no extra close/reopen. + assert.Equal(t, 1, w.Reopens()) + + now = now.Add(debounce) + + require.NoError(t, w.Write( + archive.ExportArchivedEvent{EventID: "c"}, 0, + )) + + // A write after the window elapses closes and reopens once. + assert.Equal(t, 2, w.Reopens()) +} + +func TestArchiveWriter_ExpiryPrune(t *testing.T) { + t.Parallel() + + path := filepath.Join(t.TempDir(), "archive-wh.db") + w := archive.NewExportArchiveWriter( + path, archiveTestLogger(), 0, + ) + + require.NoError(t, w.Open(0)) + + old := archive.ExportArchivedEvent{ + EventID: "old", + ArchivedAt: time.Now().Add(-2 * time.Hour), + } + fresh := archive.ExportArchivedEvent{ + EventID: "fresh", + ArchivedAt: time.Now(), + } + + require.NoError(t, w.DB().Create(&old).Error) + require.NoError(t, w.DB().Create(&fresh).Error) + + // Reopening with a one-hour expiry prunes the old row. + require.NoError(t, w.Reopen(time.Hour)) + + var got []archive.ExportArchivedEvent + + require.NoError(t, w.DB().Find(&got).Error) + require.Len(t, got, 1) + assert.Equal(t, "fresh", got[0].EventID) +} + +func TestParseArchiveExpiry(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + in string + want time.Duration + wantErr bool + }{ + {"empty config", "", 0, false}, + {"explicit never", `{"expiry":"never"}`, 0, false}, + {"empty expiry", `{"expiry":""}`, 0, false}, + {"duration", `{"expiry":"1h"}`, time.Hour, false}, + {"unparseable", `{"expiry":"nonsense"}`, 0, true}, + {"zero duration", `{"expiry":"0s"}`, 0, true}, + {"negative duration", `{"expiry":"-5h"}`, 0, true}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + got, err := archive.ParseExpiry(tc.in) + if tc.wantErr { + require.Error(t, err) + + return + } + + require.NoError(t, err) + assert.Equal(t, tc.want, got) + }) + } +} + +func TestValidateArchiveExpiry(t *testing.T) { + t.Parallel() + + valid := []string{"", "never", "1h", "720h", "30m"} + for _, in := range valid { + require.NoError(t, + archive.ValidateExpiry(in), + "expiry %q should be accepted", in, + ) + } + + invalid := []string{"nonsense", "7d", "-5h", "0s", "0"} + for _, in := range invalid { + require.Error(t, + archive.ValidateExpiry(in), + "expiry %q should be rejected", in, + ) + } +} + +// TestArchiveFileName pins the archive file name and the rules +// that make a webhook or target name safe to put in it. +func TestArchiveFileName(t *testing.T) { + t.Parallel() + + const id = "3f2a1c9e-8d4b-4c1a-9e2f-0a1b2c3d4e5f" + + cases := []struct { + name string + webhook string + target string + want string + }{ + { + "plain names", "orders", "archive", + "archive-orders-archive-" + id + ".db", + }, + { + "lowercased", "Orders", "Main Archive", + "archive-orders-main-archive-" + id + ".db", + }, + { + "a run of other characters is one dash", + `a /\..b`, "c__--d", + "archive-a-b-c-d-" + id + ".db", + }, + { + "no dash at either end", " --orders!! ", "(archive)", + "archive-orders-archive-" + id + ".db", + }, + { + "path separators", "../../etc/passwd", "a/b", + "archive-etc-passwd-a-b-" + id + ".db", + }, + { + "letters outside ASCII are dropped", + "Bestellungen Größe", "café", + "archive-bestellungen-gr-e-caf-" + id + ".db", + }, + { + "nothing left is unnamed", "", "!!!", + "archive-unnamed-unnamed-" + id + ".db", + }, + { + "cut to 40 characters", strings.Repeat("a", 50), "x", + "archive-" + strings.Repeat("a", 40) + "-x-" + id + ".db", + }, + { + "no dash left by the cut", + strings.Repeat("a", 39) + " b", "x", + "archive-" + strings.Repeat("a", 39) + "-x-" + id + ".db", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + assert.Equal( + t, tc.want, + archive.FileName(tc.webhook, tc.target, id), + ) + }) + } +} + +// TestWrite_EachTargetHasItsOwnArchive proves two +// database targets of one webhook archive into separate files. +func TestWrite_EachTargetHasItsOwnArchive(t *testing.T) { + t.Parallel() + + env := setupArchiveTest(t) + first := env.seedDatabaseTarget(t, "") + second := env.addDatabaseTarget(t, first.WebhookID, "") + + event := newEvent(`{"n":1}`) + + for _, tgt := range []*database.Target{first, second} { + require.NoError(t, env.writers.Write( + databaseTargetDelivery(event, tgt), + )) + } + + require.NotEqual( + t, env.archivePath(first), env.archivePath(second), + ) + assert.Equal( + t, []string{event.ID}, + archivedEventIDs(t, env.archivePath(first)), + ) + assert.Equal( + t, []string{event.ID}, + archivedEventIDs(t, env.archivePath(second)), + ) +} + +// TestRename_MovesTheFile proves a rename moves the archive, rows +// and all, and that later writes go to the new name. +func TestRename_MovesTheFile(t *testing.T) { + t.Parallel() + + env := setupArchiveTest(t) + tgt := env.seedDatabaseTarget(t, "") + oldPath := env.archivePath(tgt) + + first := newEvent(`{"n":1}`) + require.NoError(t, env.writers.Write(databaseTargetDelivery(first, tgt))) + require.FileExists(t, oldPath) + + require.NoError( + t, env.writers.Rename(tgt.ID, "Orders", "Long Term"), + ) + + newPath := filepath.Join( + env.dataDir, "archive-orders-long-term-"+tgt.ID+".db", + ) + + assert.NoFileExists(t, oldPath) + assert.Equal(t, []string{first.ID}, archivedEventIDs(t, newPath)) + + second := newEvent(`{"n":2}`) + require.NoError(t, env.writers.Write(databaseTargetDelivery(second, tgt))) + + assert.ElementsMatch( + t, []string{first.ID, second.ID}, + archivedEventIDs(t, newPath), + ) + assert.NoFileExists( + t, oldPath, "a write after the rename must use the new name", + ) +} + +// TestRename_NeverReplacesAFile plants a file at the new name, once +// the .db alone, once a lone -wal and once a lone -shm, and proves +// each time that the rename is refused, the planted file survives, +// and the archive keeps its name and its rows. +func TestRename_NeverReplacesAFile(t *testing.T) { + t.Parallel() + + for _, suffix := range archiveFileSuffixes() { + t.Run("planted .db"+suffix, func(t *testing.T) { + t.Parallel() + + env := setupArchiveTest(t) + tgt := env.seedDatabaseTarget(t, "") + oldPath := env.archivePath(tgt) + + first := newEvent(`{"n":1}`) + require.NoError( + t, env.writers.Write(databaseTargetDelivery(first, tgt)), + ) + + newPath := filepath.Join( + env.dataDir, "archive-orders-long-term-"+tgt.ID+".db", + ) + plantedPath := newPath + suffix + require.NoError( + t, os.WriteFile(plantedPath, []byte("planted"), 0o600), + ) + + require.ErrorIs( + t, env.writers.Rename(tgt.ID, "Orders", "Long Term"), + archive.ErrNameTaken, + ) + + //nolint:gosec // reads the file the test planted under t.TempDir() + planted, err := os.ReadFile(plantedPath) + require.NoError(t, err) + assert.Equal(t, "planted", string(planted)) + + second := newEvent(`{"n":2}`) + require.NoError( + t, env.writers.Write(databaseTargetDelivery(second, tgt)), + ) + + assert.ElementsMatch( + t, []string{first.ID, second.ID}, + archivedEventIDs(t, oldPath), + ) + }) + } +} + +// TestRename_BeforeTheNameIsSaved covers the order the handlers +// use: they rename before they save the new name, so a delivery in +// between must write under the new name although the main database +// still has the old one. It also shows that renaming an archive that +// does not exist yet is not an error. +func TestRename_BeforeTheNameIsSaved(t *testing.T) { + t.Parallel() + + env := setupArchiveTest(t) + tgt := env.seedDatabaseTarget(t, "") + + require.NoError( + t, env.writers.Rename(tgt.ID, "Orders", "Archive"), + ) + + event := newEvent(`{"n":1}`) + require.NoError(t, env.writers.Write(databaseTargetDelivery(event, tgt))) + + assert.FileExists( + t, + filepath.Join( + env.dataDir, "archive-orders-archive-"+tgt.ID+".db", + ), + ) + assert.NoFileExists(t, env.archivePath(tgt)) +} + +// TestArchiveWriter_RenameMovesSidecars proves a rename carries +// the -wal and -shm a crash can leave beside an archive no handle +// has opened since. SQLite finds them by name, so a -wal left +// behind would lose the transactions it holds. +func TestArchiveWriter_RenameMovesSidecars(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + oldPath := filepath.Join(dir, "archive-old.db") + newPath := filepath.Join(dir, "archive-new.db") + + for _, suffix := range archiveFileSuffixes() { + require.NoError( + t, os.WriteFile(oldPath+suffix, []byte(suffix), 0o600), + ) + } + + w := archive.NewExportArchiveWriter( + oldPath, archiveTestLogger(), 0, + ) + + require.NoError(t, w.Rename("archive-new.db")) + + for _, suffix := range archiveFileSuffixes() { + assert.NoFileExists(t, oldPath+suffix) + assert.FileExists(t, newPath+suffix) + } + + assert.Equal(t, newPath, w.Path()) +} + +// TestArchiveWriter_RenameMovesBackOnFailure makes the -wal fail to +// move after the .db has moved, and proves the .db is moved back, so +// the archive is never split across two names. The new name is 255 +// bytes, the longest a file name may be, so the .db can take it but +// the -wal, four bytes longer, cannot. +func TestArchiveWriter_RenameMovesBackOnFailure(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + oldPath := filepath.Join(dir, "archive-old.db") + newName := strings.Repeat("a", 252) + ".db" + + for _, suffix := range archiveFileSuffixes() { + require.NoError( + t, os.WriteFile(oldPath+suffix, []byte(suffix), 0o600), + ) + } + + w := archive.NewExportArchiveWriter( + oldPath, archiveTestLogger(), 0, + ) + + require.Error(t, w.Rename(newName)) + + for _, suffix := range archiveFileSuffixes() { + assert.FileExists(t, oldPath+suffix) + } + + assert.NoFileExists(t, filepath.Join(dir, newName)) + assert.Equal(t, oldPath, w.Path()) +} + +// TestArchiveWriter_RenameMovesBackEveryFile renames a target with a +// file without a period and a file for a month, and proves that when +// the month's file fails to move, the file already moved is moved back: +// both files are under the old name with their rows, and nothing is +// under the new name. The new name is 251 bytes, so the file without a +// period, with its -wal and -shm, can take it, but the month's file, +// eight bytes longer, cannot. +func TestArchiveWriter_RenameMovesBackEveryFile(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + oldPath := filepath.Join(dir, "archive-old.db") + monthPath := filepath.Join(dir, "archive-old-2026-03.db") + w := archive.NewExportArchiveWriter(oldPath, archiveTestLogger(), 0) + + require.NoError(t, w.WritePeriod( + archive.ExportArchivedEvent{EventID: "in-none"}, 0, "", + )) + require.NoError(t, w.WritePeriod( + archive.ExportArchivedEvent{EventID: "in-month"}, 0, "2026-03", + )) + + require.Error(t, w.Rename(strings.Repeat("a", 248)+".db")) + + assert.Equal(t, []string{"in-none"}, archivedEventIDs(t, oldPath)) + assert.Equal(t, []string{"in-month"}, archivedEventIDs(t, monthPath)) + + entries, err := os.ReadDir(dir) + require.NoError(t, err) + + for _, entry := range entries { + assert.True(t, strings.HasPrefix(entry.Name(), "archive-old"), + "%s is not under the old name", entry.Name()) + } + + assert.Equal(t, oldPath, w.Path()) +} diff --git a/internal/delivery/target_database_export.go b/internal/archive/download.go similarity index 89% rename from internal/delivery/target_database_export.go rename to internal/archive/download.go index 450c60e..a46b52e 100644 --- a/internal/delivery/target_database_export.go +++ b/internal/archive/download.go @@ -1,4 +1,4 @@ -package delivery +package archive import ( "compress/gzip" @@ -28,25 +28,25 @@ import ( const archiveTableQuery = "SELECT count(*) FROM sqlite_master " + "WHERE type = 'table' AND name = 'archived_events'" -// ArchivePath returns where a database target's archive file is: in +// Path returns where a database target's archive file is: in // the data directory, beside the webhook's event database, under the -// name ArchiveFileName gives it. -func ArchivePath( +// name FileName gives it. +func Path( dbMgr *database.WebhookDBManager, webhook *database.Webhook, target *database.Target, ) string { return filepath.Join( filepath.Dir(dbMgr.DBPath(webhook.ID)), - ArchiveFileName(webhook.Name, target.Name, target.ID), + FileName(webhook.Name, target.Name, target.ID), ) } -// ArchiveExportFileName returns the name a database target's archive +// ExportFileName returns the name a database target's archive // downloads under: // archive-WEBHOOKNAME-TARGETNAME-YYYYMMDDTHHMMSSZ.json.gz, the names -// made safe as in ArchiveFileName and the time in UTC. -func ArchiveExportFileName( +// made safe as in FileName and the time in UTC. +func ExportFileName( webhookName, targetName string, at time.Time, ) string { return "archive-" + archiveNamePart(webhookName) + "-" + @@ -54,7 +54,7 @@ func ArchiveExportFileName( at.UTC().Format("20060102T150405Z") + ".json.gz" } -// ArchiveExport is a database target's archive listed for download. It +// Export is a database target's archive listed for download. It // opens one of the target's files at a time, only when its rows are // about to be written out, and closes it before it opens the next, so // an export holds at most one file open however many the target has. @@ -66,7 +66,7 @@ func ArchiveExportFileName( // is open, and the export does not see them. SQLite cannot checkpoint // a -wal past an open snapshot, so the open file's -wal grows until the // export has written that file out. -type ArchiveExport struct { +type Export struct { // periods are the periods of the target's files when the export // was listed, "" for the file without one, in the order // archiveFiles lists them. @@ -77,7 +77,7 @@ type ArchiveExport struct { // between. lock sync.Locker - // currentPath returns the path ArchivePath gives the target under + // currentPath returns the path Path gives the target under // the names stored for it now, which a rename may have changed since // the export was listed. currentPath func() (string, error) @@ -103,11 +103,12 @@ type exportedName struct { Name string `json:"name"` } -// NewArchiveExport lists a database target's archive files for export, -// given the path ArchivePath gives it (see archiveFiles). It opens none +// NewExport lists a database target's archive files for export, +// given the path Path gives it (see archiveFiles). It opens none // of them. Its caller holds lock, which every rename of the target's -// files runs under, from reading the names path is made of until it -// returns, so the files it lists are the ones those names give. +// files runs under (Writers.Lock takes it), from reading the names +// path is made of until it returns, so the files it lists are the ones +// those names give. // // WriteGzipJSON, called without lock held, finds each file again by its // period under the path currentPath gives, holding lock while it does @@ -115,18 +116,18 @@ type exportedName struct { // file. A file that is gone by then, emptied by the sweep or moved // away, is skipped. The export never creates a file: with no files, it // has no rows. -func NewArchiveExport( +func NewExport( path string, lock sync.Locker, currentPath func() (string, error), log *slog.Logger, -) (*ArchiveExport, error) { +) (*Export, error) { files, err := archiveFiles(path) if err != nil { return nil, err } - x := &ArchiveExport{lock: lock, currentPath: currentPath, log: log} + x := &Export{lock: lock, currentPath: currentPath, log: log} for _, file := range files { x.periods = append(x.periods, file.period) @@ -199,7 +200,7 @@ func openExportFile( // closed once its rows are written, before the next is opened. When it // returns, no file is open. After an error the gzip stream is left // unfinished, so what was written does not decompress as a whole file. -func (x *ArchiveExport) WriteGzipJSON( +func (x *Export) WriteGzipJSON( ctx context.Context, w io.Writer, webhook *database.Webhook, @@ -228,7 +229,7 @@ func (x *ArchiveExport) WriteGzipJSON( // openFile finds the target's archive file for period under the path // currentPath gives now, and opens it for the export, holding x.lock // for both. For a file that is gone, the error wraps fs.ErrNotExist. -func (x *ArchiveExport) openFile( +func (x *Export) openFile( ctx context.Context, period string, ) (*exportFile, error) { x.lock.Lock() @@ -258,7 +259,7 @@ func (f *exportFile) close() error { // writeJSON writes head with archived_events added as its last key, // the rows going into it one at a time. -func (x *ArchiveExport) writeJSON( +func (x *Export) writeJSON( ctx context.Context, w io.Writer, head []byte, ) error { // head goes out without its closing brace, so that @@ -286,7 +287,7 @@ func (x *ArchiveExport) writeJSON( // writeRows writes the archived rows of each file to w, one per line, // separated by commas, opening each file in turn and closing it once // its rows are written. -func (x *ArchiveExport) writeRows(ctx context.Context, w io.Writer) error { +func (x *Export) writeRows(ctx context.Context, w io.Writer) error { sep := "\n" for _, period := range x.periods { diff --git a/internal/delivery/target_database_export_test.go b/internal/archive/download_test.go similarity index 90% rename from internal/delivery/target_database_export_test.go rename to internal/archive/download_test.go index fc6dd61..1e1a644 100644 --- a/internal/delivery/target_database_export_test.go +++ b/internal/archive/download_test.go @@ -1,4 +1,4 @@ -package delivery_test +package archive_test import ( "bufio" @@ -19,8 +19,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "sneak.berlin/go/webhooker/internal/archive" "sneak.berlin/go/webhooker/internal/database" - "sneak.berlin/go/webhooker/internal/delivery" ) // The webhook and the target the export tests' archives belong to. @@ -37,7 +37,7 @@ const binaryBody = "\xff\xfe\x00\x01binary\x80" // writeExportTo writes export to w as the archive of the export tests' // webhook and target, exported at 2026-10-02T12:03:04Z. func writeExportTo( - t *testing.T, export *delivery.ArchiveExport, w io.Writer, + t *testing.T, export *archive.Export, w io.Writer, ) error { t.Helper() @@ -57,7 +57,7 @@ func writeExportTo( // listExport lists the archive at path for export, as the archive of a // target whose names do not change. -func listExport(t *testing.T, path string) *delivery.ArchiveExport { +func listExport(t *testing.T, path string) *archive.Export { t.Helper() return newExport(t, path, &sync.Mutex{}, func() (string, error) { @@ -73,10 +73,10 @@ func newExport( path string, lock sync.Locker, currentPath func() (string, error), -) *delivery.ArchiveExport { +) *archive.Export { t.Helper() - export, err := delivery.NewArchiveExport( + export, err := archive.NewExport( path, lock, currentPath, archiveTestLogger(), ) require.NoError(t, err) @@ -96,7 +96,7 @@ func exportArchive(t *testing.T, path string) map[string]any { // decompressed and parsed. Reading to the end makes the gzip reader // check that the stream was finished. func writeExport( - t *testing.T, export *delivery.ArchiveExport, + t *testing.T, export *archive.Export, ) map[string]any { t.Helper() @@ -157,11 +157,11 @@ func TestArchiveExport_MatchesStoredRows(t *testing.T) { t.Parallel() path := filepath.Join(t.TempDir(), "archive.db") - w := delivery.NewExportArchiveWriter(path, archiveTestLogger(), 0) + w := archive.NewExportArchiveWriter(path, archiveTestLogger(), 0) bodies := []string{`{"order":1}`, "plain text", "", binaryBody} for i, body := range bodies { - require.NoError(t, w.Write(delivery.ExportArchivedEvent{ + require.NoError(t, w.Write(archive.ExportArchivedEvent{ EventID: fmt.Sprintf("ev-%d", i), WebhookID: exportWebhookID, EntrypointID: "ep-1", @@ -173,7 +173,7 @@ func TestArchiveExport_MatchesStoredRows(t *testing.T) { }, 0)) } - var stored []delivery.ExportArchivedEvent + var stored []archive.ExportArchivedEvent require.NoError(t, openArchiveDBForRead(t, path). Order("id").Find(&stored).Error) @@ -201,7 +201,7 @@ func TestArchiveExport_MatchesStoredRows(t *testing.T) { // assertExportedRow checks that ev, from an export, holds every column // of the stored row. func assertExportedRow( - t *testing.T, row delivery.ExportArchivedEvent, ev map[string]any, + t *testing.T, row archive.ExportArchivedEvent, ev map[string]any, ) { t.Helper() @@ -248,7 +248,7 @@ func TestArchiveExport_Empty(t *testing.T) { require.NoError(t, os.WriteFile(noTable, nil, 0o600)) require.NoError(t, - delivery.NewExportArchiveWriter(noRows, archiveTestLogger(), 0). + archive.NewExportArchiveWriter(noRows, archiveTestLogger(), 0). Open(0), ) @@ -284,18 +284,18 @@ func TestArchiveExport_ReadsOneSnapshot(t *testing.T) { t.Parallel() path := filepath.Join(t.TempDir(), "archive.db") - w := delivery.NewExportArchiveWriter(path, archiveTestLogger(), 0) + w := archive.NewExportArchiveWriter(path, archiveTestLogger(), 0) - require.NoError(t, w.Write(delivery.ExportArchivedEvent{EventID: "listed"}, 0)) + require.NoError(t, w.Write(archive.ExportArchivedEvent{EventID: "listed"}, 0)) opened := &unlockHook{fn: func() { - require.NoError(t, w.Write(delivery.ExportArchivedEvent{EventID: "during"}, 0)) + require.NoError(t, w.Write(archive.ExportArchivedEvent{EventID: "during"}, 0)) }} export := newExport(t, path, opened, func() (string, error) { return path, nil }) - require.NoError(t, w.Write(delivery.ExportArchivedEvent{EventID: "before-open"}, 0)) + require.NoError(t, w.Write(archive.ExportArchivedEvent{EventID: "before-open"}, 0)) assert.Equal(t, []string{"listed", "before-open"}, @@ -305,7 +305,7 @@ func TestArchiveExport_ReadsOneSnapshot(t *testing.T) { var stored int64 require.NoError(t, openArchiveDBForRead(t, path). - Model(&delivery.ExportArchivedEvent{}).Count(&stored).Error) + Model(&archive.ExportArchivedEvent{}).Count(&stored).Error) assert.Equal(t, int64(3), stored) } @@ -318,11 +318,11 @@ func TestArchiveExport_FindsFilesAfterRename(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "archive-old.db") - w := delivery.NewExportArchiveWriter(path, archiveTestLogger(), 0) + w := archive.NewExportArchiveWriter(path, archiveTestLogger(), 0) for _, period := range []string{"", dayPeriod, hourPeriod} { require.NoError(t, w.WritePeriod( - delivery.ExportArchivedEvent{EventID: "in-" + period}, 0, period, + archive.ExportArchivedEvent{EventID: "in-" + period}, 0, period, )) } @@ -353,19 +353,19 @@ func TestArchiveExport_EveryFileOldestFirst(t *testing.T) { t.Parallel() path := filepath.Join(t.TempDir(), "archive-wh.db") - w := delivery.NewExportArchiveWriter(path, archiveTestLogger(), 0) + w := archive.NewExportArchiveWriter(path, archiveTestLogger(), 0) // Written in an order that is not the export's. for _, period := range []string{nextDayPeriod, "", hourPeriod, "2026-03"} { require.NoError(t, w.WritePeriod( - delivery.ExportArchivedEvent{EventID: "in-" + period}, 0, period, + archive.ExportArchivedEvent{EventID: "in-" + period}, 0, period, )) } export := listExport(t, path) require.NoError(t, w.WritePeriod( - delivery.ExportArchivedEvent{EventID: "later"}, 0, "2026-03-06", + archive.ExportArchivedEvent{EventID: "later"}, 0, "2026-03-06", )) events := exportedEvents(t, writeExport(t, export)) @@ -434,13 +434,13 @@ func TestArchiveExport_OneFileOpenAtATime(t *testing.T) { require.NoError(t, err) path := filepath.Join(dir, "archive-wh.db") - w := delivery.NewExportArchiveWriter(path, archiveTestLogger(), 0) + w := archive.NewExportArchiveWriter(path, archiveTestLogger(), 0) random := make([]byte, 36<<10) for hour := range 24 { _, _ = rand.Read(random) - require.NoError(t, w.WritePeriod(delivery.ExportArchivedEvent{ + require.NoError(t, w.WritePeriod(archive.ExportArchivedEvent{ Body: base64.StdEncoding.EncodeToString(random), }, 0, fmt.Sprintf("2026-10-01-%02d", hour))) } @@ -489,7 +489,7 @@ func exportHeapGrowth(t *testing.T, rows, bodySize int) uint64 { t.Helper() path := filepath.Join(t.TempDir(), "archive.db") - w := delivery.NewExportArchiveWriter(path, archiveTestLogger(), 0) + w := archive.NewExportArchiveWriter(path, archiveTestLogger(), 0) // Base64 makes four characters of every three bytes. random := make([]byte, bodySize/4*3) @@ -497,7 +497,7 @@ func exportHeapGrowth(t *testing.T, rows, bodySize int) uint64 { for range rows { _, _ = rand.Read(random) - require.NoError(t, w.Write(delivery.ExportArchivedEvent{ + require.NoError(t, w.Write(archive.ExportArchivedEvent{ Body: base64.StdEncoding.EncodeToString(random), }, 0)) } @@ -565,7 +565,7 @@ func TestArchiveExportFileName(t *testing.T) { assert.Equal(t, "archive-orders-eu-long-term-archive-20261002T120304Z.json.gz", - delivery.ArchiveExportFileName( + archive.ExportFileName( exportWebhookName, exportTargetName, time.Date(2026, 10, 2, 14, 3, 4, 0, cest), ), diff --git a/internal/delivery/target_database_evict_test.go b/internal/archive/evict_test.go similarity index 81% rename from internal/delivery/target_database_evict_test.go rename to internal/archive/evict_test.go index 5022ba4..5e39c85 100644 --- a/internal/delivery/target_database_evict_test.go +++ b/internal/archive/evict_test.go @@ -1,7 +1,6 @@ -package delivery_test +package archive_test import ( - "context" "errors" "fmt" "net/http" @@ -13,8 +12,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "sneak.berlin/go/webhooker/internal/archive" "sneak.berlin/go/webhooker/internal/database" - "sneak.berlin/go/webhooker/internal/delivery" ) // deliverTo archives one event to a database target, leaving the @@ -24,12 +23,9 @@ func deliverTo( ) { t.Helper() - webhookDB := testWebhookDB(t) - event := seedEvent(t, webhookDB, `{"archived":true}`) + event := newEvent(`{"archived":true}`) - env.eng.ExportDeliverDatabase( - webhookDB, seedDatabaseTargetDelivery(t, webhookDB, event, tgt), - ) + require.NoError(t, env.writers.Write(databaseTargetDelivery(event, tgt))) } // TestEvictWebhook_ClosesAndRemovesWriter proves that evicting @@ -49,20 +45,20 @@ func TestEvictWebhook_ClosesAndRemovesWriter(t *testing.T) { deliverTo(t, env, tgt) require.True( - t, env.eng.ExportArchiveHandleOpen(tgt.ID), + t, env.writers.ExportArchiveHandleOpen(tgt.ID), "the writer should hold an open handle after a write", ) } - env.eng.EvictWebhook(first.WebhookID) + env.writers.EvictWebhook(first.WebhookID) for _, tgt := range []*database.Target{first, second} { assert.False( - t, env.eng.ExportHasArchiveWriter(tgt.ID), + t, env.writers.ExportHasArchiveWriter(tgt.ID), "eviction should remove the registry entry", ) assert.False( - t, env.eng.ExportArchiveHandleOpen(tgt.ID), + t, env.writers.ExportArchiveHandleOpen(tgt.ID), "eviction should close the archive handle", ) assert.FileExists( @@ -72,7 +68,7 @@ func TestEvictWebhook_ClosesAndRemovesWriter(t *testing.T) { } assert.True( - t, env.eng.ExportArchiveHandleOpen(other.ID), + t, env.writers.ExportArchiveHandleOpen(other.ID), "another webhook's writer must be left alone", ) } @@ -90,15 +86,15 @@ func TestEvictTarget_LeavesOtherTargets(t *testing.T) { deliverTo(t, env, doomed) deliverTo(t, env, kept) - env.eng.EvictTarget(doomed.ID) + env.writers.EvictTarget(doomed.ID) - assert.False(t, env.eng.ExportHasArchiveWriter(doomed.ID)) + assert.False(t, env.writers.ExportHasArchiveWriter(doomed.ID)) assert.FileExists( t, env.archivePath(doomed), "eviction must not delete the archive file", ) assert.True( - t, env.eng.ExportArchiveHandleOpen(kept.ID), + t, env.writers.ExportArchiveHandleOpen(kept.ID), "the other target's writer must be left alone", ) } @@ -112,21 +108,21 @@ func TestEvictWebhook_UnknownWebhookIsNoOp(t *testing.T) { env := setupArchiveTest(t) assert.NotPanics(t, func() { - env.eng.EvictWebhook("no-such-webhook") - env.eng.EvictWebhook("no-such-webhook") - env.eng.EvictTarget("no-such-target") - env.eng.EvictTarget("no-such-target") + env.writers.EvictWebhook("no-such-webhook") + env.writers.EvictWebhook("no-such-webhook") + env.writers.EvictTarget("no-such-target") + env.writers.EvictTarget("no-such-target") }) assert.False( - t, env.eng.ExportHasArchiveWriter("no-such-target"), + t, env.writers.ExportHasArchiveWriter("no-such-target"), "eviction must not create a writer", ) } // evictTestRow builds an archive row for the eviction tests. -func evictTestRow(eventID string) delivery.ExportArchivedEvent { - return delivery.ExportArchivedEvent{ +func evictTestRow(eventID string) archive.ExportArchivedEvent { + return archive.ExportArchivedEvent{ EventID: eventID, WebhookID: "wh-evict", Method: http.MethodPost, @@ -147,7 +143,7 @@ func TestEvictedWriter_WriteDoesNotReopenFile(t *testing.T) { path := filepath.Join(t.TempDir(), "archive-evicted.db") - w := delivery.NewExportArchiveWriter( + w := archive.NewExportArchiveWriter( path, archiveTestLogger(), 0, ) @@ -164,7 +160,7 @@ func TestEvictedWriter_WriteDoesNotReopenFile(t *testing.T) { err := w.Write(evictTestRow("ev-2"), 0) require.ErrorIs( - t, err, delivery.ErrExportArchiveWriterEvicted, + t, err, archive.ErrExportArchiveWriterEvicted, "an evicted writer must refuse writes", ) assert.NoFileExists( @@ -187,7 +183,7 @@ func TestEvictedWriter_SweepDoesNotReopenFile(t *testing.T) { path := filepath.Join(t.TempDir(), "archive-evicted.db") - w := delivery.NewExportArchiveWriter( + w := archive.NewExportArchiveWriter( path, archiveTestLogger(), 0, ) @@ -199,7 +195,7 @@ func TestEvictedWriter_SweepDoesNotReopenFile(t *testing.T) { err := w.SweepExpired(time.Hour) require.ErrorIs( - t, err, delivery.ErrExportArchiveWriterEvicted, + t, err, archive.ErrExportArchiveWriterEvicted, "an evicted writer must refuse an idle sweep", ) assert.False( @@ -230,7 +226,7 @@ const racingWriteGoroutines = 4 // against a hang, not a timing assumption: the first write after // the eviction is refused. func startRacingWrites( - w *delivery.ExportArchiveWriter, + w *archive.ExportArchiveWriter, ) *racingWrites { r := &racingWrites{ started: make(chan struct{}, racingWriteGoroutines), @@ -276,7 +272,7 @@ func (r *racingWrites) record(err error) { r.mu.Lock() defer r.mu.Unlock() - if errors.Is(err, delivery.ErrExportArchiveWriterEvicted) { + if errors.Is(err, archive.ErrExportArchiveWriterEvicted) { r.sawEvicted = true return @@ -320,7 +316,7 @@ func TestEvictWebhook_RacingWriteDoesNotReopenHandle( // eviction is about to detach. deliverTo(t, env, tgt) - w := env.eng.ExportArchiveWriterFor(tgt.ID) + w := env.writers.ExportArchiveWriterFor(tgt.ID) require.NotNil(t, w) require.True(t, w.HandleOpen()) @@ -330,7 +326,7 @@ func TestEvictWebhook_RacingWriteDoesNotReopenHandle( // eviction has to contend for the writer's mutex. race.awaitFirstWrite() - env.eng.EvictWebhook(tgt.WebhookID) + env.writers.EvictWebhook(tgt.WebhookID) sawEvicted, otherErr := race.wait() @@ -345,7 +341,7 @@ func TestEvictWebhook_RacingWriteDoesNotReopenHandle( "been evicted", ) assert.False( - t, env.eng.ExportHasArchiveWriter(tgt.ID), + t, env.writers.ExportHasArchiveWriter(tgt.ID), "the registry entry must stay gone", ) } @@ -362,25 +358,25 @@ func TestEvictWebhook_LaterDeliveryRecreatesWriter(t *testing.T) { tgt := env.seedDatabaseTarget(t, "") deliverTo(t, env, tgt) - require.True(t, env.eng.ExportHasArchiveWriter(tgt.ID)) + require.True(t, env.writers.ExportHasArchiveWriter(tgt.ID)) - env.eng.EvictWebhook(tgt.WebhookID) + env.writers.EvictWebhook(tgt.WebhookID) // A fresh delivery for the same target gets a brand new // writer from the registry, so archiving keeps working. deliverTo(t, env, tgt) assert.True( - t, env.eng.ExportHasArchiveWriter(tgt.ID), + t, env.writers.ExportHasArchiveWriter(tgt.ID), "a later delivery should recreate the writer", ) } -// TestEngineStop_WriteAfterStopIsRefused proves the engine's stop -// closes each archive writer the way deleting its webhook does: a -// write that reaches a writer after the stop is refused, reopens -// nothing and adds no row. -func TestEngineStop_WriteAfterStopIsRefused(t *testing.T) { +// TestEvictAll_WriteAfterIsRefused proves that EvictAll, which the +// delivery engine calls when it stops, closes each archive writer the +// way deleting its webhook does: a write that reaches a writer after +// it is refused, reopens nothing and adds no row. +func TestEvictAll_WriteAfterIsRefused(t *testing.T) { t.Parallel() env := setupArchiveTest(t) @@ -388,16 +384,16 @@ func TestEngineStop_WriteAfterStopIsRefused(t *testing.T) { deliverTo(t, env, tgt) - w := env.eng.ExportArchiveWriterFor(tgt.ID) + w := env.writers.ExportArchiveWriterFor(tgt.ID) require.NotNil(t, w) require.True(t, w.HandleOpen()) - require.NoError(t, env.eng.ExportStop(context.Background())) + env.writers.EvictAll() err := w.Write(evictTestRow("ev-after-stop"), 0) require.ErrorIs( - t, err, delivery.ErrExportArchiveWriterEvicted, + t, err, archive.ErrExportArchiveWriterEvicted, "a write after the stop must be refused", ) assert.False( @@ -405,7 +401,7 @@ func TestEngineStop_WriteAfterStopIsRefused(t *testing.T) { "a refused write must not reopen the archive", ) assert.False( - t, env.eng.ExportHasArchiveWriter(tgt.ID), + t, env.writers.ExportHasArchiveWriter(tgt.ID), "the stop should empty the registry", ) diff --git a/internal/archive/export_test.go b/internal/archive/export_test.go new file mode 100644 index 0000000..0bb8648 --- /dev/null +++ b/internal/archive/export_test.go @@ -0,0 +1,280 @@ +package archive + +import ( + "context" + "log/slog" + "time" + + "go.uber.org/fx" + "gorm.io/gorm" + "sneak.berlin/go/webhooker/internal/database" +) + +// ErrExportArchiveWriterEvicted exposes the sentinel returned by +// an evicted archive writer. It carries the Err prefix rather +// than this file's usual Export one because it is a sentinel +// error. +var ErrExportArchiveWriterEvicted = errArchiveWriterEvicted + +// ExportArchivedEvent aliases the archive row type so black-box +// tests can construct and read archive rows. +type ExportArchivedEvent = archivedEvent + +// ExportArchiveWriter wraps an archiveWriter so black-box tests +// can exercise the archive file mechanics. +type ExportArchiveWriter struct { + w *archiveWriter +} + +// NewExportArchiveWriter builds an archive writer for tests, +// optionally overriding the reopen debounce (a non-positive +// debounce keeps the production default). +func NewExportArchiveWriter( + path string, log *slog.Logger, debounce time.Duration, +) *ExportArchiveWriter { + w := newArchiveWriter(path, log) + if debounce > 0 { + w.debounce = debounce + } + + return &ExportArchiveWriter{w: w} +} + +// Write archives a row through the writer, into the file named +// without a period. +func (e *ExportArchiveWriter) Write( + row ExportArchivedEvent, expiry time.Duration, +) error { + return e.w.write(row, expiry, "") +} + +// WritePeriod archives a row through the writer, into the file for +// period. +func (e *ExportArchiveWriter) WritePeriod( + row ExportArchivedEvent, expiry time.Duration, period string, +) error { + return e.w.write(row, expiry, period) +} + +// Open opens the archive file, pruning when expiry is positive. +func (e *ExportArchiveWriter) Open(expiry time.Duration) error { + return e.w.open(e.w.path, expiry) +} + +// Reopen closes and reopens the archive file. +func (e *ExportArchiveWriter) Reopen( + expiry time.Duration, +) error { + return e.w.reopen(e.w.path, expiry) +} + +// SetNow replaces the clock the writer measures its reopen +// debounce on. +func (e *ExportArchiveWriter) SetNow(now func() time.Time) { + e.w.now = now +} + +// Reopens reports how many times the file has been opened. +func (e *ExportArchiveWriter) Reopens() int { + return e.w.reopens +} + +// DB returns the writer's current open handle for row +// inspection in tests. +func (e *ExportArchiveWriter) DB() *gorm.DB { + return e.w.db +} + +// Path returns the archive file the writer owns. +func (e *ExportArchiveWriter) Path() string { + return e.w.path +} + +// OpenExisting opens the archive without permitting creation, +// the way the idle sweep does. +func (e *ExportArchiveWriter) OpenExisting( + expiry time.Duration, +) error { + return e.w.openMode(e.w.path, archiveModeExisting, expiry) +} + +// SweepExpired runs an idle sweep of the archive. +func (e *ExportArchiveWriter) SweepExpired( + expiry time.Duration, +) error { + return e.w.sweepExpired(expiry) +} + +// Evict marks the writer evicted and closes its handle, exactly +// as leaving the registry does. +func (e *ExportArchiveWriter) Evict() { + e.w.evict() +} + +// Rename gives the archive file a new name in the same directory, +// as a rename of the webhook or target does. +func (e *ExportArchiveWriter) Rename(name string) error { + return e.w.rename(name) +} + +// HandleOpen reports whether the writer currently holds an open +// archive handle. +func (e *ExportArchiveWriter) HandleOpen() bool { + e.w.mu.Lock() + defer e.w.mu.Unlock() + + return e.w.db != nil +} + +// Same reports whether both wrappers refer to the very same +// underlying archive writer, so a test can prove a registry entry +// is the writer it was handed rather than a replacement. +func (e *ExportArchiveWriter) Same( + other *ExportArchiveWriter, +) bool { + return other != nil && e.w == other.w +} + +// ExportArchiveWriterFor returns the archive writer the registry +// currently caches for a database target, or nil when none is +// cached. It never creates one, so a test can hold a reference to +// the very writer an eviction is about to detach. +func (ws *Writers) ExportArchiveWriterFor( + targetID string, +) *ExportArchiveWriter { + ws.mu.Lock() + defer ws.mu.Unlock() + + w, ok := ws.writers[targetID] + if !ok { + return nil + } + + return &ExportArchiveWriter{w: w} +} + +// ExportHasArchiveWriter reports whether the registry currently +// caches an archive writer for a target. +func (ws *Writers) ExportHasArchiveWriter( + targetID string, +) bool { + ws.mu.Lock() + defer ws.mu.Unlock() + + _, ok := ws.writers[targetID] + + return ok +} + +// ExportArchiveHandleOpen reports whether the cached archive +// writer for a target holds an open database handle. It +// returns false when no writer is cached. +func (ws *Writers) ExportArchiveHandleOpen( + targetID string, +) bool { + ws.mu.Lock() + w, ok := ws.writers[targetID] + ws.mu.Unlock() + + if !ok { + return false + } + + w.mu.Lock() + defer w.mu.Unlock() + + return w.db != nil +} + +// ExportEnsureArchiveWriter creates (if needed) and returns the +// archive file path of the cached writer for a target, so a +// test can prime the registry the way a delivery would. +func (ws *Writers) ExportEnsureArchiveWriter( + targetID string, +) (string, error) { + w, err := ws.writerFor(targetID) + if err != nil { + return "", err + } + + return w.path, nil +} + +// ExportSweepWriterFor takes a target's registry writer exactly +// as the idle sweep does, reporting whether the sweep had to +// create the entry. It lets a test drive the registry through the +// sweep's own entry point instead of choreographing goroutines. +func (ws *Writers) ExportSweepWriterFor( + targetID string, +) (*ExportArchiveWriter, bool, error) { + w, created, err := ws.sweepWriterFor(targetID) + if err != nil { + return nil, false, err + } + + return &ExportArchiveWriter{w: w}, created, nil +} + +// ExportReleaseSweepWriter releases a sweep-created registry entry +// exactly as a finished sweep does. +func (ws *Writers) ExportReleaseSweepWriter( + targetID string, w *ExportArchiveWriter, +) { + ws.releaseSweepWriter(targetID, w.w) +} + +// NewTestSweeper builds a Sweeper backed by the given main +// database and archive writers, without the fx lifecycle. +// Intended for tests. +func NewTestSweeper( + db *database.Database, + writers *Writers, + log *slog.Logger, +) *Sweeper { + return &Sweeper{ + db: db, + writers: writers, + log: log, + interval: time.Hour, + } +} + +// ExportSweep runs a single archive sweep synchronously for +// tests. +func (s *Sweeper) ExportSweep(ctx context.Context) { + s.sweep(ctx) +} + +// ExportStart starts the sweeper's background loop for tests. +func (s *Sweeper) ExportStart() { + s.start() +} + +// ExportRegisterHooks registers the sweeper's real fx lifecycle +// hooks on a lifecycle supplied by a test, so a test can drive +// the exact OnStart/OnStop functions the application runs and +// hand OnStart the kind of context fx actually supplies. +func (s *Sweeper) ExportRegisterHooks(lc fx.Lifecycle) { + s.registerHooks(lc) +} + +// ExportStop stops the sweeper's background loop for tests. +func (s *Sweeper) ExportStop(ctx context.Context) error { + return s.stop(ctx) +} + +// ExportWedgeLoop adds a goroutine to the sweeper's WaitGroup +// that never observes cancellation and returns only when release +// is closed. It stands in for a prune stuck on a locked archive. +func (s *Sweeper) ExportWedgeLoop( + release <-chan struct{}, +) { + s.wg.Go(func() { + <-release + }) +} + +// ExportSetInterval overrides the sweep interval for tests. +func (s *Sweeper) ExportSetInterval(d time.Duration) { + s.interval = d +} diff --git a/internal/delivery/target_database_rotation.go b/internal/archive/rotation.go similarity index 85% rename from internal/delivery/target_database_rotation.go rename to internal/archive/rotation.go index e16180e..fea54ba 100644 --- a/internal/delivery/target_database_rotation.go +++ b/internal/archive/rotation.go @@ -1,4 +1,4 @@ -package delivery +package archive import ( "encoding/json" @@ -41,10 +41,10 @@ type archiveFile struct { period string } -// ValidateArchiveRotation checks a user-supplied archive rotation for +// ValidateRotation checks a user-supplied archive rotation for // a database target: empty or none (both meaning one file), monthly, // daily or hourly. -func ValidateArchiveRotation(rotation string) error { +func ValidateRotation(rotation string) error { switch rotation { case "", archiveRotationNone, archiveRotationMonthly, archiveRotationDaily, archiveRotationHourly: @@ -54,21 +54,21 @@ func ValidateArchiveRotation(rotation string) error { } } -// parseArchiveRotation reads the rotation from a database target's +// ParseRotation reads the rotation from a database target's // config JSON. An empty config or an empty rotation is none. -func parseArchiveRotation(configJSON string) (string, error) { +func ParseRotation(configJSON string) (string, error) { if configJSON == "" { return archiveRotationNone, nil } - var cfg databaseTargetConfig + var cfg DatabaseTargetConfig err := json.Unmarshal([]byte(configJSON), &cfg) if err != nil { return "", fmt.Errorf("parsing database target config: %w", err) } - err = ValidateArchiveRotation(cfg.Rotation) + err = ValidateRotation(cfg.Rotation) if err != nil { return "", err } @@ -97,7 +97,7 @@ func archivePeriod(rotation string, receivedAt time.Time) string { } // archivePeriodPath returns the path of a database target's archive -// file for a period: path, as ArchivePath gives it, with "-" and the +// file for a period: path, as Path gives it, with "-" and the // period put before its ".db". The period "" gives path itself. func archivePeriodPath(path, period string) string { if period == "" { @@ -107,28 +107,28 @@ func archivePeriodPath(path, period string) string { return strings.TrimSuffix(path, ".db") + "-" + period + ".db" } -// ArchivePathAt returns the archive file a database target writes an -// event received at receivedAt to: ArchivePath's file, with the +// PathAt returns the archive file a database target writes an +// event received at receivedAt to: Path's file, with the // period in its name when the target rotates. -func ArchivePathAt( +func PathAt( dbMgr *database.WebhookDBManager, webhook *database.Webhook, target *database.Target, receivedAt time.Time, ) (string, error) { - rotation, err := parseArchiveRotation(target.Config) + rotation, err := ParseRotation(target.Config) if err != nil { return "", err } return archivePeriodPath( - ArchivePath(dbMgr, webhook, target), + Path(dbMgr, webhook, target), archivePeriod(rotation, receivedAt), ), nil } // archiveFiles lists the database target's archive files that exist, -// given the path ArchivePath gives it: the file at path, then each +// given the path Path gives it: the file at path, then each // file archivePeriodPath names for path and a period, oldest period // first. Which rotation wrote a file does not matter, so the files of // an earlier rotation setting are listed too. diff --git a/internal/delivery/target_database_rotation_test.go b/internal/archive/rotation_test.go similarity index 87% rename from internal/delivery/target_database_rotation_test.go rename to internal/archive/rotation_test.go index 667a23b..d7f338b 100644 --- a/internal/delivery/target_database_rotation_test.go +++ b/internal/archive/rotation_test.go @@ -1,4 +1,4 @@ -package delivery_test +package archive_test import ( "os" @@ -9,9 +9,9 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "sneak.berlin/go/webhooker/internal/archive" "sneak.berlin/go/webhooker/internal/database" "sneak.berlin/go/webhooker/internal/database/databasetest" - "sneak.berlin/go/webhooker/internal/delivery" ) // The archive rotations, and the configs of a daily target and of one @@ -40,32 +40,30 @@ func periodPath(path, period string) string { return strings.TrimSuffix(path, ".db") + "-" + period + ".db" } -// deliverReceivedAt delivers to a database target an event whose -// receive time is receivedAt, and returns the event's id. The receive -// time is what decides a rotated archive's file, so setting it is how -// these tests move the clock across a period boundary. +// deliverReceivedAt archives, as a delivery to a database target +// does, an event whose receive time is receivedAt, and returns the +// event's id. The receive time is what decides a rotated archive's +// file, so setting it is how these tests move the clock across a +// period boundary. func (env *archiveEnv) deliverReceivedAt( t *testing.T, tgt *database.Target, receivedAt time.Time, ) string { t.Helper() - webhookDB := testWebhookDB(t) - event := seedEvent(t, webhookDB, `{"n":1}`) + event := newEvent(`{"n":1}`) event.CreatedAt = receivedAt - env.eng.ExportDeliverDatabase( - webhookDB, seedDatabaseTargetDelivery(t, webhookDB, event, tgt), - ) + require.NoError(t, env.writers.Write(databaseTargetDelivery(event, tgt))) return event.ID } -// TestDeliverDatabase_RotatesAtEachPeriodBoundary delivers, for each +// TestWrite_RotatesAtEachPeriodBoundary writes, for each // rotation, an event received in the last second of a period and one // received in the first second of the next, and checks each lands in // the file named for its own period, in UTC. Rotation none keeps both // in the one file. -func TestDeliverDatabase_RotatesAtEachPeriodBoundary(t *testing.T) { +func TestWrite_RotatesAtEachPeriodBoundary(t *testing.T) { t.Parallel() berlin := time.FixedZone("CEST", 2*60*60) @@ -139,10 +137,10 @@ func TestDeliverDatabase_RotatesAtEachPeriodBoundary(t *testing.T) { }) } -// TestDeliverDatabase_EventWithoutReceiveTime proves an event whose +// TestWrite_EventWithoutReceiveTime proves an event whose // receive time is not known goes to the file for the time it is // archived, rather than to one for the year 1. -func TestDeliverDatabase_EventWithoutReceiveTime(t *testing.T) { +func TestWrite_EventWithoutReceiveTime(t *testing.T) { t.Parallel() env := setupArchiveTest(t) @@ -164,10 +162,10 @@ func TestDeliverDatabase_EventWithoutReceiveTime(t *testing.T) { assert.NoFileExists(t, periodPath(path, "0001-01")) } -// TestDeliverDatabase_RotationChangeKeepsOldFiles changes a target's +// TestWrite_RotationChangeKeepsOldFiles changes a target's // rotation from none to daily between two events, and checks the // second goes to the daily file while the first stays where it was. -func TestDeliverDatabase_RotationChangeKeepsOldFiles(t *testing.T) { +func TestWrite_RotationChangeKeepsOldFiles(t *testing.T) { t.Parallel() env := setupArchiveTest(t) @@ -237,7 +235,7 @@ func TestRename_MovesEveryFile(t *testing.T) { first := env.deliverReceivedAt(t, tgt, day) second := env.deliverReceivedAt(t, tgt, day.Add(24*time.Hour)) - require.NoError(t, env.eng.Rename(tgt.ID, "Orders", "Long Term")) + require.NoError(t, env.writers.Rename(tgt.ID, "Orders", "Long Term")) newPath := filepath.Join( env.dataDir, "archive-orders-long-term-"+tgt.ID+".db", @@ -283,8 +281,8 @@ func TestRename_NeverReplacesARotatedFile(t *testing.T) { require.NoError(t, os.WriteFile(planted, []byte("planted"), 0o600)) require.ErrorIs( - t, env.eng.Rename(tgt.ID, "Orders", "Long Term"), - delivery.ErrArchiveNameTaken, + t, env.writers.Rename(tgt.ID, "Orders", "Long Term"), + archive.ErrNameTaken, ) assert.FileExists(t, periodPath(oldPath, dayPeriod)) @@ -292,7 +290,7 @@ func TestRename_NeverReplacesARotatedFile(t *testing.T) { assert.NoFileExists(t, periodPath(newPath, dayPeriod)) } -// TestStatArchive_EveryFile proves StatArchive counts and adds up every +// TestStatArchive_EveryFile proves Stat counts and adds up every // one of a target's files, takes the latest write of any of them, and // leaves out files whose names only look like the target's. func TestStatArchive_EveryFile(t *testing.T) { @@ -324,7 +322,7 @@ func TestStatArchive_EveryFile(t *testing.T) { os.WriteFile(filepath.Join(dir, other), []byte("x"), 0o600)) } - got, err := delivery.StatArchive(path) + got, err := archive.Stat(path) require.NoError(t, err) assert.Equal(t, len(files), got.Files) assert.Equal(t, size, got.Size) @@ -357,7 +355,7 @@ func TestArchivePathAt(t *testing.T) { Config: config, } - got, err := delivery.ArchivePathAt(dbMgr, webhook, target, at) + got, err := archive.PathAt(dbMgr, webhook, target, at) require.NoError(t, err, config) assert.Equal(t, filepath.Join( @@ -367,7 +365,7 @@ func TestArchivePathAt(t *testing.T) { ) } - _, err := delivery.ArchivePathAt(dbMgr, webhook, &database.Target{ + _, err := archive.PathAt(dbMgr, webhook, &database.Target{ Config: weeklyConfig, }, at) require.Error(t, err) @@ -381,10 +379,10 @@ func TestValidateArchiveRotation(t *testing.T) { for _, ok := range []string{ "", rotationNone, rotationMonthly, rotationDaily, rotationHourly, } { - require.NoError(t, delivery.ValidateArchiveRotation(ok), ok) + require.NoError(t, archive.ValidateRotation(ok), ok) } for _, bad := range []string{"weekly", "Daily", "hourly "} { - require.Error(t, delivery.ValidateArchiveRotation(bad), bad) + require.Error(t, archive.ValidateRotation(bad), bad) } } diff --git a/internal/delivery/archive_sweeper.go b/internal/archive/sweeper.go similarity index 85% rename from internal/delivery/archive_sweeper.go rename to internal/archive/sweeper.go index 1a6b87a..34338df 100644 --- a/internal/delivery/archive_sweeper.go +++ b/internal/archive/sweeper.go @@ -1,4 +1,4 @@ -package delivery +package archive import ( "context" @@ -15,18 +15,18 @@ import ( "sneak.berlin/go/webhooker/internal/logger" ) -// ArchiveSweeperParams holds the fx dependencies for the -// ArchiveSweeper. -type ArchiveSweeperParams struct { +// SweeperParams holds the fx dependencies for the +// Sweeper. +type SweeperParams struct { fx.In Config *config.Config Database *database.Database - Engine *Engine + Writers *Writers Logger *logger.Logger } -// ArchiveSweeper periodically prunes expired rows from the +// Sweeper periodically prunes expired rows from the // archive databases of database targets that carry a positive // expiry. // @@ -40,9 +40,9 @@ type ArchiveSweeperParams struct { // It reuses Config.RetentionSweepInterval rather than // introducing a second interval: this is a retention sweep with // the same semantics as the event retention reaper. -type ArchiveSweeper struct { +type Sweeper struct { db *database.Database - eng *Engine + writers *Writers log *slog.Logger interval time.Duration @@ -54,16 +54,16 @@ type ArchiveSweeper struct { wg sync.WaitGroup } -// NewArchiveSweeper creates the archive sweeper and registers +// NewSweeper creates the archive sweeper and registers // its fx lifecycle hooks. The background sweep loop starts on // OnStart and stops cleanly on OnStop via context cancellation. -func NewArchiveSweeper( +func NewSweeper( lc fx.Lifecycle, - params ArchiveSweeperParams, -) *ArchiveSweeper { - s := &ArchiveSweeper{ + params SweeperParams, +) *Sweeper { + s := &Sweeper{ db: params.Database, - eng: params.Engine, + writers: params.Writers, log: params.Logger.Get(), interval: params.Config.RetentionSweepInterval, } @@ -77,7 +77,7 @@ func NewArchiveSweeper( // lifecycle. The start hook's context is deliberately ignored // (see start for why the background loop must not inherit it); // the stop hook's context is honoured (see stop). -func (s *ArchiveSweeper) registerHooks(lc fx.Lifecycle) { +func (s *Sweeper) registerHooks(lc fx.Lifecycle) { lc.Append(fx.Hook{ //nolint:contextcheck // Not passing the hook context is // the point: see start. @@ -103,7 +103,7 @@ func (s *ArchiveSweeper) registerHooks(lc fx.Lifecycle) { // goroutine must outlive the startup phase, so its lifetime is // bounded by OnStop instead: stop cancels this context and waits // on the WaitGroup. -func (s *ArchiveSweeper) start() { +func (s *Sweeper) start() { ctx, cancel := context.WithCancel(context.Background()) s.cancel = cancel @@ -121,7 +121,7 @@ func (s *ArchiveSweeper) start() { // exit, bounded by the stop hook's context: a prune wedged on a // locked archive must not hang the process past fx's stop // timeout. -func (s *ArchiveSweeper) stop(ctx context.Context) error { +func (s *Sweeper) stop(ctx context.Context) error { s.log.Info("archive sweeper stopping") if s.cancel != nil { @@ -140,7 +140,7 @@ func (s *ArchiveSweeper) stop(ctx context.Context) error { return nil } -func (s *ArchiveSweeper) run(ctx context.Context) { +func (s *Sweeper) run(ctx context.Context) { defer s.wg.Done() ticker := time.NewTicker(s.interval) @@ -164,7 +164,7 @@ func (s *ArchiveSweeper) run(ctx context.Context) { // A failure for one target is logged and the sweep continues, // matching how the write path already treats a prune error as // non-fatal. -func (s *ArchiveSweeper) sweep(ctx context.Context) { +func (s *Sweeper) sweep(ctx context.Context) { var targets []database.Target err := s.db.DB(). @@ -202,8 +202,8 @@ func (s *ArchiveSweeper) sweep(ctx context.Context) { // sweepTarget prunes the archive of a single database target. // A missing, empty, or "never" expiry parses as a zero duration // and is skipped entirely, so those archives are never pruned. -func (s *ArchiveSweeper) sweepTarget(target *database.Target) { - expiry, err := parseArchiveExpiry(target.Config) +func (s *Sweeper) sweepTarget(target *database.Target) { + expiry, err := ParseExpiry(target.Config) if err != nil { s.log.Error( "archive sweep: invalid database target config", @@ -219,11 +219,11 @@ func (s *ArchiveSweeper) sweepTarget(target *database.Target) { return } - if s.eng == nil || s.eng.dbTarget == nil { + if s.writers == nil { return } - err = s.eng.dbTarget.sweepArchive(target.ID, expiry) + err = s.writers.sweepArchive(target.ID, expiry) if err == nil { return } diff --git a/internal/delivery/archive_sweeper_test.go b/internal/archive/sweeper_test.go similarity index 85% rename from internal/delivery/archive_sweeper_test.go rename to internal/archive/sweeper_test.go index 46a1f3a..b9a3d4d 100644 --- a/internal/delivery/archive_sweeper_test.go +++ b/internal/archive/sweeper_test.go @@ -1,4 +1,4 @@ -package delivery_test +package archive_test import ( "bytes" @@ -16,14 +16,15 @@ import ( "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/fx" "gorm.io/driver/sqlite" "gorm.io/gorm" "gorm.io/gorm/clause" _ "modernc.org/sqlite" // Pure Go SQLite driver. + "sneak.berlin/go/webhooker/internal/archive" "sneak.berlin/go/webhooker/internal/database" "sneak.berlin/go/webhooker/internal/database/databasetest" "sneak.berlin/go/webhooker/internal/db/migrations" - "sneak.berlin/go/webhooker/internal/delivery" "sneak.berlin/go/webhooker/internal/gormlog" ) @@ -37,6 +38,18 @@ const ( // sweepConcurrentWrites is how many deliveries the // concurrent write-plus-sweep test races against the sweep. sweepConcurrentWrites = 20 + + // hookStopTimeout bounds how long a lifecycle test waits for + // the sweeper's OnStop hook to return before declaring the + // shutdown hung. + hookStopTimeout = 10 * time.Second + + // wedgeStopTimeout is the stop timeout a wedged-shutdown test + // hands OnStop, standing in for fx's StopTimeout. The test + // asserts only that the hook returns at all, and allows it + // hookStopTimeout — forty times this budget — to do so, so no + // assertion here races the wall clock. + wedgeStopTimeout = 250 * time.Millisecond ) // archiveTestWebhookName is the name of every webhook @@ -45,12 +58,12 @@ const ( const archiveTestWebhookName = "Sweep Test!" // archiveEnv bundles the pieces an archive test drives: a main -// configuration database holding webhooks and targets, a delivery -// engine owning the archive writer registry, the archive sweeper, -// and the data directory the archive files live in. +// configuration database holding webhooks and targets, the archive +// writers, the archive sweeper, and the data directory the archive +// files live in. type archiveEnv struct { - sweeper *delivery.ArchiveSweeper - eng *delivery.Engine + sweeper *archive.Sweeper + writers *archive.Writers mainDB *database.Database dataDir string } @@ -66,30 +79,28 @@ func setupArchiveTest(t *testing.T) *archiveEnv { t.Cleanup(func() { _ = mainDB.Close() }) - eng := delivery.NewTestEngineWithDB( - mainDB, - databasetest.NewWebhookDBManager(t, dataDir), - log, - &http.Client{Timeout: 5 * time.Second}, - 1, - ) + writers := archive.NewWriters(archive.WritersParams{ + Database: mainDB, + DBManager: databasetest.NewWebhookDBManager(t, dataDir), + Logger: log, + }) return &archiveEnv{ - sweeper: delivery.NewTestArchiveSweeper( - mainDB, eng, log, + sweeper: archive.NewTestSweeper( + mainDB, writers, log, ), - eng: eng, + writers: writers, mainDB: mainDB, dataDir: dataDir, } } -// archivePath returns where the engine keeps a database target's +// archivePath returns where the writers keep a database target's // archive file, for the names seedDatabaseTarget gave it. func (env *archiveEnv) archivePath(tgt *database.Target) string { return filepath.Join( env.dataDir, - delivery.ArchiveFileName( + archive.FileName( archiveTestWebhookName, tgt.Name, tgt.ID, ), ) @@ -177,7 +188,7 @@ func seedArchiveFile( require.NoError(t, migrations.Apply(gdb, migrations.Archive)) for i, at := range archivedAt { - row := delivery.ExportArchivedEvent{ + row := archive.ExportArchivedEvent{ EventID: fmt.Sprintf("ev-%d", i), WebhookID: webhookID, Method: http.MethodPost, @@ -197,7 +208,7 @@ func archivedEventIDs( ) []string { t.Helper() - var rows []delivery.ExportArchivedEvent + var rows []archive.ExportArchivedEvent rdb := openArchiveDBForRead(t, path) require.NoError(t, rdb.Order("event_id").Find(&rows).Error) @@ -234,7 +245,7 @@ func countArchivedRows(path string) (int64, error) { var count int64 - err = gdb.Model(&delivery.ExportArchivedEvent{}). + err = gdb.Model(&archive.ExportArchivedEvent{}). Count(&count).Error if err != nil { return 0, err @@ -243,6 +254,55 @@ func countArchivedRows(path string) (int64, error) { return count, nil } +// recordingLifecycle is a minimal fx.Lifecycle that records the +// hooks a component registers, so a test can invoke the real +// OnStart/OnStop functions with a context of its choosing. +type recordingLifecycle struct { + hooks []fx.Hook +} + +func (l *recordingLifecycle) Append(h fx.Hook) { + l.hooks = append(l.hooks, h) +} + +// requireStopHookExpires drives hook.OnStop with a stop context +// that expires while a wedged goroutine is still running, and +// requires the hook to return the deadline error naming +// component instead of blocking on the WaitGroup forever. +func requireStopHookExpires( + t *testing.T, hook fx.Hook, component string, +) { + t.Helper() + + stopCtx, cancel := context.WithTimeout( + context.Background(), wedgeStopTimeout, + ) + defer cancel() + + var stopErr error + + stopped := make(chan struct{}) + + go func() { + defer close(stopped) + + stopErr = hook.OnStop(stopCtx) + }() + + select { + case <-stopped: + case <-time.After(hookStopTimeout): + t.Fatal( + "OnStop did not return: it discarded the stop " + + "context and is waiting on a wedged goroutine " + + "that will never observe cancellation", + ) + } + + require.ErrorIs(t, stopErr, context.DeadlineExceeded) + require.ErrorContains(t, stopErr, component) +} + // TestArchiveSweeper_LoopOutlivesStartHookContext is the // regression test for a sweeper that never swept. fx calls // OnStart with a context carrying the application's start @@ -326,16 +386,16 @@ func TestArchiveSweep_DoesNotResurrectEvictedWriter( // the deletion path does. The target row is deliberately left // in place: this is the tick that listed the target before // the deletion committed. - _, err := env.eng.ExportEnsureArchiveWriter(tgt.ID) + _, err := env.writers.ExportEnsureArchiveWriter(tgt.ID) require.NoError(t, err) - env.eng.EvictTarget(tgt.ID) - require.False(t, env.eng.ExportHasArchiveWriter(tgt.ID)) + env.writers.EvictTarget(tgt.ID) + require.False(t, env.writers.ExportHasArchiveWriter(tgt.ID)) env.sweeper.ExportSweep(context.Background()) assert.False( - t, env.eng.ExportHasArchiveWriter(tgt.ID), + t, env.writers.ExportHasArchiveWriter(tgt.ID), "a sweep must never re-register a writer for a target "+ "whose registry entry has already been released", ) @@ -358,7 +418,7 @@ func TestArchiveSweep_LeavesNoRegistryEntry(t *testing.T) { time.Now().Add(-time.Minute), ) - require.False(t, env.eng.ExportHasArchiveWriter(tgt.ID)) + require.False(t, env.writers.ExportHasArchiveWriter(tgt.ID)) env.sweeper.ExportSweep(context.Background()) @@ -367,7 +427,7 @@ func TestArchiveSweep_LeavesNoRegistryEntry(t *testing.T) { "the sweep must still prune an idle archive", ) assert.False( - t, env.eng.ExportHasArchiveWriter(tgt.ID), + t, env.writers.ExportHasArchiveWriter(tgt.ID), "the sweep must release the registry entry it created", ) } @@ -389,24 +449,22 @@ func TestArchiveSweep_KeepsWriterAdoptedByDelivery( t, tgt, time.Now().Add(-48*time.Hour), ) - webhookDB := testWebhookDB(t) - event := seedEvent(t, webhookDB, `{"n":1}`) - d := seedDatabaseTargetDelivery(t, webhookDB, event, tgt) + d := databaseTargetDelivery(newEvent(`{"n":1}`), tgt) env.sweeper.ExportSweep(context.Background()) - require.False(t, env.eng.ExportHasArchiveWriter(tgt.ID)) + require.False(t, env.writers.ExportHasArchiveWriter(tgt.ID)) - env.eng.ExportDeliverDatabase(webhookDB, d) + require.NoError(t, env.writers.Write(d)) assert.True( - t, env.eng.ExportHasArchiveWriter(tgt.ID), + t, env.writers.ExportHasArchiveWriter(tgt.ID), "a delivery's writer must stay registered", ) env.sweeper.ExportSweep(context.Background()) assert.True( - t, env.eng.ExportHasArchiveWriter(tgt.ID), + t, env.writers.ExportHasArchiveWriter(tgt.ID), "a sweep must not drop a writer a delivery owns", ) } @@ -445,7 +503,7 @@ func TestArchiveSweep_KeepsWriterAdoptedDuringSweep( t, tgt, time.Now().Add(-48*time.Hour), ) - sweepWriter, created, err := env.eng.ExportSweepWriterFor( + sweepWriter, created, err := env.writers.ExportSweepWriterFor( tgt.ID, ) require.NoError(t, err) @@ -455,36 +513,34 @@ func TestArchiveSweep_KeepsWriterAdoptedDuringSweep( ) // The delivery lands mid-sweep and adopts the entry. - webhookDB := testWebhookDB(t) - event := seedEvent(t, webhookDB, `{"n":1}`) - d := seedDatabaseTargetDelivery(t, webhookDB, event, tgt) + d := databaseTargetDelivery(newEvent(`{"n":1}`), tgt) - env.eng.ExportDeliverDatabase(webhookDB, d) + require.NoError(t, env.writers.Write(d)) - adopted := env.eng.ExportArchiveWriterFor(tgt.ID) + adopted := env.writers.ExportArchiveWriterFor(tgt.ID) require.NotNil(t, adopted) require.True( t, sweepWriter.Same(adopted), "the delivery must have adopted the sweep's writer", ) require.True( - t, env.eng.ExportArchiveHandleOpen(tgt.ID), + t, env.writers.ExportArchiveHandleOpen(tgt.ID), "the delivery leaves the archive handle open", ) // The sweep finishes. - env.eng.ExportReleaseSweepWriter(tgt.ID, sweepWriter) + env.writers.ExportReleaseSweepWriter(tgt.ID, sweepWriter) require.True( - t, env.eng.ExportHasArchiveWriter(tgt.ID), + t, env.writers.ExportHasArchiveWriter(tgt.ID), "a writer adopted by a delivery during a sweep must "+ "stay registered, or its open handle is unreachable", ) - env.eng.EvictTarget(tgt.ID) + env.writers.EvictTarget(tgt.ID) assert.False( - t, env.eng.ExportHasArchiveWriter(tgt.ID), + t, env.writers.ExportHasArchiveWriter(tgt.ID), "the adopted writer must still be evictable", ) assert.False( @@ -548,7 +604,7 @@ func TestArchiveSweep_OpenExistingDoesNotCreateFile( dir := t.TempDir() path := filepath.Join(dir, "archive-absent.db") - w := delivery.NewExportArchiveWriter( + w := archive.NewExportArchiveWriter( path, archiveTestLogger(), 0, ) @@ -619,7 +675,7 @@ func TestArchiveSweep_LeavesArchiveClosed(t *testing.T) { t, tgt, time.Now().Add(-48*time.Hour), ) - w := delivery.NewExportArchiveWriter( + w := archive.NewExportArchiveWriter( path, archiveTestLogger(), 0, ) @@ -659,25 +715,23 @@ func TestArchiveSweep_ClosesHandleOfRegisteredWriter( t, tgt, time.Now().Add(-48*time.Hour), ) - webhookDB := testWebhookDB(t) - event := seedEvent(t, webhookDB, `{"n":1}`) - d := seedDatabaseTargetDelivery(t, webhookDB, event, tgt) + d := databaseTargetDelivery(newEvent(`{"n":1}`), tgt) - env.eng.ExportDeliverDatabase(webhookDB, d) + require.NoError(t, env.writers.Write(d)) require.True( - t, env.eng.ExportArchiveHandleOpen(tgt.ID), + t, env.writers.ExportArchiveHandleOpen(tgt.ID), "the delivery must leave the archive handle open", ) env.sweeper.ExportSweep(context.Background()) require.True( - t, env.eng.ExportHasArchiveWriter(tgt.ID), + t, env.writers.ExportHasArchiveWriter(tgt.ID), "the delivery's registry entry must survive the sweep", ) assert.False( - t, env.eng.ExportArchiveHandleOpen(tgt.ID), + t, env.writers.ExportArchiveHandleOpen(tgt.ID), "the sweep must leave the archive closed", ) } @@ -693,7 +747,7 @@ func TestArchiveSweep_ClosesHandleBeforeReopening(t *testing.T) { path := filepath.Join(t.TempDir(), "archive.db") - w := delivery.NewExportArchiveWriter( + w := archive.NewExportArchiveWriter( path, archiveTestLogger(), 0, ) @@ -721,8 +775,8 @@ func TestArchiveSweep_CancelledSweepLogsNoError(t *testing.T) { var errorLines bytes.Buffer - sweeper := delivery.NewTestArchiveSweeper( - env.mainDB, env.eng, + sweeper := archive.NewTestSweeper( + env.mainDB, env.writers, slog.New(slog.NewTextHandler( &errorLines, &slog.HandlerOptions{Level: slog.LevelError}, @@ -766,7 +820,7 @@ func TestArchiveSweep_NeverExpiryUntouched(t *testing.T) { "config %q must keep rows forever", configJSON, ) assert.False( - t, env.eng.ExportHasArchiveWriter(tgt.ID), + t, env.writers.ExportHasArchiveWriter(tgt.ID), "config %q must leave no registry entry behind", configJSON, ) @@ -831,7 +885,7 @@ func archiveTableExists(t *testing.T, path string) bool { return openArchiveDBForRead(t, path). Migrator(). - HasTable(&delivery.ExportArchivedEvent{}) + HasTable(&archive.ExportArchivedEvent{}) } // TestArchiveSweep_DoesNotCreateArchiveFile proves the sweep @@ -858,7 +912,7 @@ func TestArchiveSweep_DoesNotCreateArchiveFile(t *testing.T) { } assert.False( - t, env.eng.ExportHasArchiveWriter(tgt.ID), + t, env.writers.ExportHasArchiveWriter(tgt.ID), "the sweep must leave no registry entry behind", ) } @@ -876,7 +930,7 @@ func TestArchiveSweep_DoesNotCreateAfterWriterExists( tgt := env.seedDatabaseTarget(t, `{"expiry":"1h"}`) - path, err := env.eng.ExportEnsureArchiveWriter(tgt.ID) + path, err := env.writers.ExportEnsureArchiveWriter(tgt.ID) require.NoError(t, err) require.NoFileExists(t, path) @@ -923,21 +977,14 @@ func TestArchiveSweep_ConcurrentWrites(t *testing.T) { tgt := env.seedDatabaseTarget(t, `{"expiry":"1h"}`) - webhookDB := testWebhookDB(t) - - // The deliveries are seeded up front, on the test's own - // goroutine: the seed helpers assert, and testify assertions - // must not run off the test goroutine. deliveries := make( []*database.Delivery, 0, sweepConcurrentWrites, ) for range sweepConcurrentWrites { - event := seedEvent(t, webhookDB, `{"n":1}`) - deliveries = append( deliveries, - seedDatabaseTargetDelivery(t, webhookDB, event, tgt), + databaseTargetDelivery(newEvent(`{"n":1}`), tgt), ) } @@ -949,7 +996,7 @@ func TestArchiveSweep_ConcurrentWrites(t *testing.T) { defer wg.Done() for _, d := range deliveries { - env.eng.ExportDeliverDatabase(webhookDB, d) + _ = env.writers.Write(d) } }() diff --git a/internal/delivery/target_database_archive.go b/internal/archive/writer.go similarity index 92% rename from internal/delivery/target_database_archive.go rename to internal/archive/writer.go index 519af99..624ccae 100644 --- a/internal/delivery/target_database_archive.go +++ b/internal/archive/writer.go @@ -1,4 +1,4 @@ -package delivery +package archive import ( "encoding/json" @@ -18,9 +18,9 @@ import ( "sneak.berlin/go/webhooker/internal/gormlog" ) -// archiveExpiryNever is the expiry sentinel (and default) that +// ExpiryNever is the expiry sentinel (and default) that // disables pruning so archived rows are kept forever. -const archiveExpiryNever = "never" +const ExpiryNever = "never" // archiveReopenDebounce bounds how often an archive file is // closed and reopened. After each write the handle is closed @@ -71,17 +71,17 @@ var ( "archive writer has been evicted", ) - // ErrArchiveNameTaken is returned when an archive cannot be + // ErrNameTaken is returned when an archive cannot be // renamed because a file already has the new name. That file may // be an archive with rows of its own, so it is never replaced. - ErrArchiveNameTaken = errors.New( + ErrNameTaken = errors.New( "a file already has the archive's new name", ) ) -// databaseTargetConfig is the optional per-target JSON config +// DatabaseTargetConfig is the optional per-target JSON config // for a database (archive) target. -type databaseTargetConfig struct { +type DatabaseTargetConfig struct { // Expiry is a Go duration (e.g. "720h") after which // archived rows are pruned, or "never" (the default) to // keep them forever. @@ -112,21 +112,21 @@ type archivedEvent struct { ArchivedAt time.Time `gorm:"index"` } -// parseArchiveExpiry reads the optional expiry from a database +// ParseExpiry reads the optional expiry from a database // target's config JSON. An empty config, an empty expiry, or // the literal "never" all mean keep forever, returned as a zero // duration. Any other value must parse as a positive Go // duration; a set-but-invalid value (unparseable, zero, or // negative) is an error rather than a silent default, matching -// ValidateArchiveExpiry at target creation. -func parseArchiveExpiry( +// ValidateExpiry at target creation. +func ParseExpiry( configJSON string, ) (time.Duration, error) { if configJSON == "" { return 0, nil } - var cfg databaseTargetConfig + var cfg DatabaseTargetConfig err := json.Unmarshal([]byte(configJSON), &cfg) if err != nil { @@ -135,7 +135,7 @@ func parseArchiveExpiry( ) } - if cfg.Expiry == "" || cfg.Expiry == archiveExpiryNever { + if cfg.Expiry == "" || cfg.Expiry == ExpiryNever { return 0, nil } @@ -155,14 +155,14 @@ func parseArchiveExpiry( return dur, nil } -// ValidateArchiveExpiry checks a user-supplied archive expiry +// ValidateExpiry checks a user-supplied archive expiry // for a database target at configuration time. Valid values are // empty, "never" (both meaning keep forever), or a positive Go // duration such as "720h". Anything else is an error, so a bad // expiry is rejected when the target is created rather than // failing every subsequent delivery. -func ValidateArchiveExpiry(expiry string) error { - if expiry == "" || expiry == archiveExpiryNever { +func ValidateExpiry(expiry string) error { + if expiry == "" || expiry == ExpiryNever { return nil } @@ -171,7 +171,7 @@ func ValidateArchiveExpiry(expiry string) error { return fmt.Errorf( "expiry must be %q or a Go duration "+ "such as \"720h\": %w", - archiveExpiryNever, err, + ExpiryNever, err, ) } @@ -194,7 +194,7 @@ func ValidateArchiveExpiry(expiry string) error { type archiveWriter struct { mu sync.Mutex - // path is the target's archive file as ArchivePath names it. A + // path is the target's archive file as Path names it. A // target that rotates writes to the files archivePeriodPath names // for path and a period instead. path string @@ -232,7 +232,7 @@ type archiveWriter struct { // registry proper. // // Unlike every other field here it is guarded by - // databaseTarget.mu, not by this writer's mu: it describes the + // Writers.mu, not by this writer's mu: it describes the // registry entry rather than the file. sweepOwned bool } @@ -499,7 +499,7 @@ func (w *archiveWriter) sweepFile( // the next write creates its file under the new name. // // If a file already has one of the new names, nothing is moved and -// the error is ErrArchiveNameTaken. If one file fails to move, those +// the error is ErrNameTaken. If one file fails to move, those // already moved are moved back before the error is returned, so the // archive is never split across two names. func (w *archiveWriter) rename(name string) error { @@ -537,7 +537,7 @@ func (w *archiveWriter) rename(name string) error { for _, taken := range to { if fileExists(taken) { return fmt.Errorf( - "%w: %s", ErrArchiveNameTaken, filepath.Base(taken), + "%w: %s", ErrNameTaken, filepath.Base(taken), ) } } @@ -590,7 +590,7 @@ func (w *archiveWriter) evict() { // each row's archived time. It runs on every (re)open, so a // steadily written archive is swept by its own write traffic. An // archive that goes idle receives no further reopens, which is -// why ArchiveSweeper exists to drive sweepExpired on a timer. +// why Sweeper exists to drive sweepExpired on a timer. // Failures are logged, not fatal: a prune error must not stop // archiving. func (w *archiveWriter) prune(expiry time.Duration) { @@ -617,9 +617,9 @@ func (w *archiveWriter) prune(expiry time.Duration) { } } -// ArchiveFileInfo is what the metadata of a database target's archive +// FileInfo is what the metadata of a database target's archive // files says about them. -type ArchiveFileInfo struct { +type FileInfo struct { // Files counts the files. Files int @@ -631,18 +631,18 @@ type ArchiveFileInfo struct { Written time.Time } -// StatArchive reads the metadata of a database target's archive -// files, given the path ArchivePath gives it (see archiveFiles), and +// Stat reads the metadata of a database target's archive +// files, given the path Path gives it (see archiveFiles), and // of their -wal, without opening them. With no files, which is so // before the first write and after the operator moved them away, the // error wraps fs.ErrNotExist. -func StatArchive(path string) (ArchiveFileInfo, error) { +func Stat(path string) (FileInfo, error) { files, err := archiveFiles(path) if err != nil { - return ArchiveFileInfo{}, err + return FileInfo{}, err } - var info ArchiveFileInfo + var info FileInfo for _, file := range files { db, err := os.Stat(file.path) @@ -651,7 +651,7 @@ func StatArchive(path string) (ArchiveFileInfo, error) { } if err != nil { - return ArchiveFileInfo{}, err + return FileInfo{}, err } info.Files++ @@ -667,7 +667,7 @@ func StatArchive(path string) (ArchiveFileInfo, error) { } if err != nil { - return ArchiveFileInfo{}, err + return FileInfo{}, err } info.Size += wal.Size() @@ -678,7 +678,7 @@ func StatArchive(path string) (ArchiveFileInfo, error) { } if info.Files == 0 { - return ArchiveFileInfo{}, fmt.Errorf( + return FileInfo{}, fmt.Errorf( "no archive file for %s: %w", path, fs.ErrNotExist, ) } diff --git a/internal/delivery/target_database_archive_gormlog_test.go b/internal/archive/writer_gormlog_test.go similarity index 93% rename from internal/delivery/target_database_archive_gormlog_test.go rename to internal/archive/writer_gormlog_test.go index 1644f8c..5ada804 100644 --- a/internal/delivery/target_database_archive_gormlog_test.go +++ b/internal/archive/writer_gormlog_test.go @@ -1,4 +1,4 @@ -package delivery_test +package archive_test import ( "bytes" @@ -14,7 +14,7 @@ import ( "github.com/stretchr/testify/require" "gorm.io/gorm" gormlogger "gorm.io/gorm/logger" - "sneak.berlin/go/webhooker/internal/delivery" + "sneak.berlin/go/webhooker/internal/archive" "sneak.berlin/go/webhooker/internal/middleware" ) @@ -57,7 +57,7 @@ func (g *gormDefaultBuf) String() string { // without exporting production code to carry it, and a logging // detector is not worth a production symbol. What it detects is the // third gorm.Open in this service, at -// internal/delivery/target_database_archive.go — the archive writer, +// internal/archive/writer.go — the archive writer, // whose type is unexported, so nothing outside this package can drive // it. func captureArchiveGORMDefault(t *testing.T) *gormDefaultBuf { @@ -85,7 +85,7 @@ func captureArchiveGORMDefault(t *testing.T) *gormDefaultBuf { // writer's gorm.Open to the adapter. // // Restore a bare &gorm.Config{} at -// internal/delivery/target_database_archive.go and this fails: the +// internal/archive/writer.go and this fails: the // default logger prints the fully interpolated SELECT on every // ErrRecordNotFound, so the client-chosen event id below arrives whole // and unbounded on stdout, answering to no level the operator set. @@ -100,7 +100,7 @@ func TestArchiveWriter_NeverUsesGORMsDefaultLogger(t *testing.T) { gormDefault := captureArchiveGORMDefault(t) - w := delivery.NewExportArchiveWriter( + w := archive.NewExportArchiveWriter( filepath.Join(t.TempDir(), "archive.db"), slog.New(slog.NewTextHandler( &captured, &slog.HandlerOptions{Level: slog.LevelDebug}, @@ -118,7 +118,7 @@ func TestArchiveWriter_NeverUsesGORMsDefaultLogger(t *testing.T) { value := strings.Repeat("\x01", archiveGORMFillBytes) + archiveGORMTailMarker - var row delivery.ExportArchivedEvent + var row archive.ExportArchivedEvent err := w.DB().Where("event_id = ?", value).First(&row).Error require.ErrorIs(t, err, gorm.ErrRecordNotFound) diff --git a/internal/database/sqlite_mode_test.go b/internal/database/sqlite_mode_test.go index f75c3fa..70ca676 100644 --- a/internal/database/sqlite_mode_test.go +++ b/internal/database/sqlite_mode_test.go @@ -137,7 +137,7 @@ func TestPerWebhookEventDatabaseFilesAreOwnerOnly(t *testing.T) { } // TestArchiveDatabaseFilesAreOwnerOnly covers the archive-*.db tier. -// internal/delivery builds that path and opens it through OpenSQLite, +// internal/archive builds that path and opens it through OpenSQLite, // the same single open path exercised here, so the mode is settled for // all three tiers in one place. func TestArchiveDatabaseFilesAreOwnerOnly(t *testing.T) { diff --git a/internal/db/migrations/archive/001_schema.sql b/internal/db/migrations/archive/001_schema.sql index ceff586..7f1e403 100644 --- a/internal/db/migrations/archive/001_schema.sql +++ b/internal/db/migrations/archive/001_schema.sql @@ -1,5 +1,5 @@ -- Migration 001: the schema of a database target's archive file. The --- archivedEvent model in internal/delivery must agree with it. +-- archivedEvent model in internal/archive must agree with it. CREATE TABLE archived_events ( id integer PRIMARY KEY AUTOINCREMENT, diff --git a/internal/delivery/engine.go b/internal/delivery/engine.go index cde4128..b566809 100644 --- a/internal/delivery/engine.go +++ b/internal/delivery/engine.go @@ -13,6 +13,7 @@ import ( "go.uber.org/fx" "gorm.io/gorm" + "sneak.berlin/go/webhooker/internal/archive" "sneak.berlin/go/webhooker/internal/database" "sneak.berlin/go/webhooker/internal/globals" "sneak.berlin/go/webhooker/internal/lifecycle" @@ -124,31 +125,11 @@ type Notifier interface { Notify(tasks []Task) } -// Archives is how the handlers keep the database targets' archive -// files in step with the configuration. Deleting a webhook or a -// target releases the cached archive writers, whose open file -// handles would otherwise outlive them; renaming one renames the -// archive files, which are named for the webhook and the target -// (see ArchiveFileName). -// -// It is deliberately separate from Notifier: archiving lifecycle -// is not notification, and a small interface keeps the handlers -// package free of any dependency on the engine's internals while -// staying trivially fakeable in tests. -// -// Neither eviction deletes an archive file. Both are idempotent -// and are no-ops for a webhook or target with no engine state. -type Archives interface { - EvictWebhook(webhookID string) - EvictTarget(targetID string) - Rename(targetID, webhookName, targetName string) error -} - // CircuitBreakers is how the handlers read a target's circuit // breaker, so the webhook page and the event log can say that -// deliveries to the target are paused and until when. Like Archives, -// it keeps the handlers free of the engine's internals and is -// trivially faked in tests. +// deliveries to the target are paused and until when. It keeps the +// handlers free of the engine's internals and is trivially faked in +// tests. type CircuitBreakers interface { StateAndCooldown(targetID string) (CircuitState, time.Duration) } @@ -164,6 +145,7 @@ type EngineParams struct { Logger *logger.Logger SSRFGuard *Guard Metrics *metrics.Set + Archives *archive.Writers } // Engine processes queued deliveries in the background @@ -202,9 +184,10 @@ type Engine struct { httpTarget *httpTarget slackTarget *slackTarget - // dbTarget is retained so the engine can reach the archive - // writer registry for eviction, renames and the idle sweep. - dbTarget *databaseTarget + // archives is the database targets' archive writers. The + // database target writes each event through them, and stop + // closes them. + archives *archive.Writers // inflight is the set of deliveries this engine currently owns. // Recovery and the sweeps re-dispatch only what it does not @@ -228,6 +211,7 @@ func New( workers: defaultWorkers, version: params.Globals.Version, mtr: params.Metrics, + archives: params.Archives, } e.initTargets(&http.Client{ @@ -272,47 +256,6 @@ func (e *Engine) Notify(tasks []Task) { } } -// EvictWebhook implements Archives. The cached archive writer of -// every database target of the webhook is dropped from the -// registry and its file handle closed. The archive files -// themselves are left on disk — they are long-term storage the -// operator owns. -func (e *Engine) EvictWebhook(webhookID string) { - if e.dbTarget == nil { - return - } - - e.dbTarget.evictWebhook(webhookID) -} - -// EvictTarget implements Archives. It is EvictWebhook for a single -// database target, and leaves the archive file on disk the same -// way. -func (e *Engine) EvictTarget(targetID string) { - if e.dbTarget == nil { - return - } - - e.dbTarget.evict(targetID) -} - -// Rename implements Archives. It renames every one of a database -// target's archive files to ArchiveFileName(webhookName, targetName, -// targetID), each keeping the period in its name, under the lock the -// target's archive writes and the idle sweep take. It never replaces -// a file: if one already has a new name, the error is -// ErrArchiveNameTaken. The caller renames before it saves the new -// name: see databaseTarget.rename. -func (e *Engine) Rename( - targetID, webhookName, targetName string, -) error { - if e.dbTarget == nil { - return nil - } - - return e.dbTarget.rename(targetID, webhookName, targetName) -} - // StateAndCooldown implements CircuitBreakers. It returns the state of // the target's circuit breaker and, while the breaker is open, what is // left of its cooldown; the cooldown is zero once that has passed and @@ -466,7 +409,7 @@ func (e *Engine) stop(ctx context.Context) error { return err } - e.dbTarget.evictAll() + e.archives.EvictAll() e.log.Info("delivery engine stopped") diff --git a/internal/delivery/engine_lifecycle_test.go b/internal/delivery/engine_lifecycle_test.go index 1775fc8..36cf5d5 100644 --- a/internal/delivery/engine_lifecycle_test.go +++ b/internal/delivery/engine_lifecycle_test.go @@ -427,9 +427,9 @@ func TestEngine_StopLeavesLastRetryAttemptRetrying(t *testing.T) { // deliverToArchive gives the setup's webhook a database target, // runs one delivery to it through the running engine, and returns -// the target's ID and archive file path. The archive writer holds -// the file open afterwards. -func deliverToArchive(t *testing.T, s iSetup) (string, string) { +// the target's archive file path. The archive writer holds the file +// open afterwards. +func deliverToArchive(t *testing.T, s iSetup) string { t.Helper() iCreateWebhook(t, s.MainDB, s.WebhookID, "hook") @@ -452,7 +452,7 @@ func deliverToArchive(t *testing.T, s iSetup) (string, string) { iWaitForDelivered(t, s.WebhookDB, deliveryID) - return tgt.ID, filepath.Join( + return filepath.Join( filepath.Dir(s.DBMgr.DBPath(s.WebhookID)), "archive-hook-archive-"+tgt.ID+".db", ) @@ -471,7 +471,7 @@ func TestEngine_StopHookClosesArchives(t *testing.T) { lc := startEngineViaHook(t, s.Engine) - _, path := deliverToArchive(t, s) + path := deliverToArchive(t, s) require.FileExists( t, path+"-wal", "an open archive should have a -wal for the stop to remove", @@ -505,21 +505,23 @@ func TestEngine_StopHookTimeoutLeavesArchivesOpen(t *testing.T) { lc := startEngineViaHook(t, s.Engine) - targetID, _ := deliverToArchive(t, s) + path := deliverToArchive(t, s) release := make(chan struct{}) t.Cleanup(func() { close(release) - s.Engine.EvictWebhook(s.WebhookID) + s.Engine.ExportArchives().EvictWebhook(s.WebhookID) }) s.Engine.ExportWedgeWorker(release) requireStopHookExpires(t, lc.hooks[0], "delivery engine") - require.True( - t, s.Engine.ExportArchiveHandleOpen(targetID), + // Closing the last handle on an archive removes its -wal, so a + // -wal still there is a writer still open. + require.FileExists( + t, path+"-wal", "a stop that timed out must not close archive writers", ) } diff --git a/internal/delivery/export_test.go b/internal/delivery/export_test.go index 51a3e61..9256445 100644 --- a/internal/delivery/export_test.go +++ b/internal/delivery/export_test.go @@ -12,16 +12,11 @@ import ( "github.com/prometheus/client_golang/prometheus" "go.uber.org/fx" "gorm.io/gorm" + "sneak.berlin/go/webhooker/internal/archive" "sneak.berlin/go/webhooker/internal/database" "sneak.berlin/go/webhooker/internal/metrics" ) -// ErrExportArchiveWriterEvicted exposes the sentinel returned by -// an evicted archive writer. It carries the Err prefix rather -// than this file's usual Export one because it is a sentinel -// error. -var ErrExportArchiveWriterEvicted = errArchiveWriterEvicted - // Exported constants for test access. const ( ExportDeliveryChannelSize = deliveryChannelSize @@ -399,7 +394,8 @@ func (e *Engine) ExportRetryCh() chan Task { } // NewTestEngine creates an Engine for unit tests without -// database dependencies. +// database dependencies. Its archive writers have no data +// directory, so a delivery to a database target fails. func NewTestEngine( log *slog.Logger, client *http.Client, @@ -411,6 +407,7 @@ func NewTestEngine( retryCh: make(chan Task, retryChannelSize), workers: workers, mtr: metrics.New(prometheus.NewRegistry()), + archives: archive.NewWriters(archive.WritersParams{Logger: log}), } e.initTargets(client) @@ -423,9 +420,10 @@ func NewTestEngineSmallRetry( log *slog.Logger, ) *Engine { e := &Engine{ - log: log, - retryCh: make(chan Task, 1), - mtr: metrics.New(prometheus.NewRegistry()), + log: log, + retryCh: make(chan Task, 1), + mtr: metrics.New(prometheus.NewRegistry()), + archives: archive.NewWriters(archive.WritersParams{Logger: log}), } e.initTargets(nil) @@ -433,7 +431,8 @@ func NewTestEngineSmallRetry( } // NewTestEngineWithDB creates an Engine with a real -// database and dbManager for integration tests. +// database and dbManager for integration tests, and archive +// writers of its own over the same two. func NewTestEngineWithDB( db *database.Database, dbMgr *database.WebhookDBManager, @@ -449,12 +448,22 @@ func NewTestEngineWithDB( retryCh: make(chan Task, retryChannelSize), workers: workers, mtr: metrics.New(prometheus.NewRegistry()), + archives: archive.NewWriters(archive.WritersParams{ + Database: db, + DBManager: dbMgr, + Logger: log, + }), } e.initTargets(client) return e } +// ExportArchives returns the engine's archive writers. +func (e *Engine) ExportArchives() *archive.Writers { + return e.archives +} + // ExportSetMetrics substitutes the engine's metric set, so a test can // assert on collectors registered on a registry it holds. func (e *Engine) ExportSetMetrics(mtr *metrics.Set) { @@ -477,273 +486,3 @@ func NewTestCircuitBreaker( cooldown: cooldown, } } - -// ExportArchivedEvent aliases the archive row type so black-box -// tests can construct and read archive rows. -type ExportArchivedEvent = archivedEvent - -// ExportArchiveWriter wraps an archiveWriter so black-box tests -// can exercise the archive file mechanics. -type ExportArchiveWriter struct { - w *archiveWriter -} - -// NewExportArchiveWriter builds an archive writer for tests, -// optionally overriding the reopen debounce (a non-positive -// debounce keeps the production default). -func NewExportArchiveWriter( - path string, log *slog.Logger, debounce time.Duration, -) *ExportArchiveWriter { - w := newArchiveWriter(path, log) - if debounce > 0 { - w.debounce = debounce - } - - return &ExportArchiveWriter{w: w} -} - -// Write archives a row through the writer, into the file named -// without a period. -func (e *ExportArchiveWriter) Write( - row ExportArchivedEvent, expiry time.Duration, -) error { - return e.w.write(row, expiry, "") -} - -// WritePeriod archives a row through the writer, into the file for -// period. -func (e *ExportArchiveWriter) WritePeriod( - row ExportArchivedEvent, expiry time.Duration, period string, -) error { - return e.w.write(row, expiry, period) -} - -// Open opens the archive file, pruning when expiry is positive. -func (e *ExportArchiveWriter) Open(expiry time.Duration) error { - return e.w.open(e.w.path, expiry) -} - -// Reopen closes and reopens the archive file. -func (e *ExportArchiveWriter) Reopen( - expiry time.Duration, -) error { - return e.w.reopen(e.w.path, expiry) -} - -// SetNow replaces the clock the writer measures its reopen -// debounce on. -func (e *ExportArchiveWriter) SetNow(now func() time.Time) { - e.w.now = now -} - -// Reopens reports how many times the file has been opened. -func (e *ExportArchiveWriter) Reopens() int { - return e.w.reopens -} - -// DB returns the writer's current open handle for row -// inspection in tests. -func (e *ExportArchiveWriter) DB() *gorm.DB { - return e.w.db -} - -// Path returns the archive file the writer owns. -func (e *ExportArchiveWriter) Path() string { - return e.w.path -} - -// OpenExisting opens the archive without permitting creation, -// the way the idle sweep does. -func (e *ExportArchiveWriter) OpenExisting( - expiry time.Duration, -) error { - return e.w.openMode(e.w.path, archiveModeExisting, expiry) -} - -// SweepExpired runs an idle sweep of the archive. -func (e *ExportArchiveWriter) SweepExpired( - expiry time.Duration, -) error { - return e.w.sweepExpired(expiry) -} - -// Evict marks the writer evicted and closes its handle, exactly -// as leaving the registry does. -func (e *ExportArchiveWriter) Evict() { - e.w.evict() -} - -// Rename gives the archive file a new name in the same directory, -// as a rename of the webhook or target does. -func (e *ExportArchiveWriter) Rename(name string) error { - return e.w.rename(name) -} - -// HandleOpen reports whether the writer currently holds an open -// archive handle. -func (e *ExportArchiveWriter) HandleOpen() bool { - e.w.mu.Lock() - defer e.w.mu.Unlock() - - return e.w.db != nil -} - -// Same reports whether both wrappers refer to the very same -// underlying archive writer, so a test can prove a registry entry -// is the writer it was handed rather than a replacement. -func (e *ExportArchiveWriter) Same( - other *ExportArchiveWriter, -) bool { - return other != nil && e.w == other.w -} - -// ExportArchiveWriterFor returns the archive writer the registry -// currently caches for a database target, or nil when none is -// cached. It never creates one, so a test can hold a reference to -// the very writer an eviction is about to detach. -func (e *Engine) ExportArchiveWriterFor( - targetID string, -) *ExportArchiveWriter { - e.dbTarget.mu.Lock() - defer e.dbTarget.mu.Unlock() - - w, ok := e.dbTarget.writers[targetID] - if !ok { - return nil - } - - return &ExportArchiveWriter{w: w} -} - -// ExportHasArchiveWriter reports whether the database target -// type currently caches an archive writer for a target. -func (e *Engine) ExportHasArchiveWriter( - targetID string, -) bool { - e.dbTarget.mu.Lock() - defer e.dbTarget.mu.Unlock() - - _, ok := e.dbTarget.writers[targetID] - - return ok -} - -// ExportArchiveHandleOpen reports whether the cached archive -// writer for a target holds an open database handle. It -// returns false when no writer is cached. -func (e *Engine) ExportArchiveHandleOpen( - targetID string, -) bool { - e.dbTarget.mu.Lock() - w, ok := e.dbTarget.writers[targetID] - e.dbTarget.mu.Unlock() - - if !ok { - return false - } - - w.mu.Lock() - defer w.mu.Unlock() - - return w.db != nil -} - -// ExportEnsureArchiveWriter creates (if needed) and returns the -// archive file path of the cached writer for a target, so a -// test can prime the registry the way a delivery would. -func (e *Engine) ExportEnsureArchiveWriter( - targetID string, -) (string, error) { - w, err := e.dbTarget.writerFor(targetID) - if err != nil { - return "", err - } - - return w.path, nil -} - -// ExportSweepWriterFor takes a target's registry writer exactly -// as the idle sweep does, reporting whether the sweep had to -// create the entry. It lets a test drive the registry through the -// sweep's own entry point instead of choreographing goroutines. -func (e *Engine) ExportSweepWriterFor( - targetID string, -) (*ExportArchiveWriter, bool, error) { - w, created, err := e.dbTarget.sweepWriterFor(targetID) - if err != nil { - return nil, false, err - } - - return &ExportArchiveWriter{w: w}, created, nil -} - -// ExportReleaseSweepWriter releases a sweep-created registry entry -// exactly as a finished sweep does. -func (e *Engine) ExportReleaseSweepWriter( - targetID string, w *ExportArchiveWriter, -) { - e.dbTarget.releaseSweepWriter(targetID, w.w) -} - -// NewTestArchiveSweeper builds an ArchiveSweeper backed by the -// given main database and engine, without the fx lifecycle. -// Intended for tests. -func NewTestArchiveSweeper( - db *database.Database, - eng *Engine, - log *slog.Logger, -) *ArchiveSweeper { - return &ArchiveSweeper{ - db: db, - eng: eng, - log: log, - interval: time.Hour, - } -} - -// ExportSweep runs a single archive sweep synchronously for -// tests. -func (s *ArchiveSweeper) ExportSweep(ctx context.Context) { - s.sweep(ctx) -} - -// ExportStart starts the sweeper's background loop for tests. -func (s *ArchiveSweeper) ExportStart() { - s.start() -} - -// ExportRegisterHooks registers the sweeper's real fx lifecycle -// hooks on a lifecycle supplied by a test, so a test can drive -// the exact OnStart/OnStop functions the application runs and -// hand OnStart the kind of context fx actually supplies. -func (s *ArchiveSweeper) ExportRegisterHooks(lc fx.Lifecycle) { - s.registerHooks(lc) -} - -// ExportStop stops the sweeper's background loop for tests. -func (s *ArchiveSweeper) ExportStop(ctx context.Context) error { - return s.stop(ctx) -} - -// ExportWedgeLoop adds a goroutine to the sweeper's WaitGroup -// that never observes cancellation and returns only when release -// is closed. It stands in for a prune stuck on a locked archive. -func (s *ArchiveSweeper) ExportWedgeLoop( - release <-chan struct{}, -) { - s.wg.Go(func() { - <-release - }) -} - -// ExportSetInterval overrides the sweep interval for tests. -func (s *ArchiveSweeper) ExportSetInterval(d time.Duration) { - s.interval = d -} - -// ExportParseArchiveExpiry exposes parseArchiveExpiry. -func ExportParseArchiveExpiry( - configJSON string, -) (time.Duration, error) { - return parseArchiveExpiry(configJSON) -} diff --git a/internal/delivery/inflight_test.go b/internal/delivery/inflight_test.go index c50e3c0..f6ec27e 100644 --- a/internal/delivery/inflight_test.go +++ b/internal/delivery/inflight_test.go @@ -2,8 +2,6 @@ package delivery_test import ( "context" - "path/filepath" - "strings" "sync" "testing" "time" @@ -66,33 +64,6 @@ func fDrain(e *delivery.Engine) []delivery.Task { } } -// TestArchiveHandleIsWAL closes the last gap in the durability -// evidence: the main and per-webhook tiers each assert their journal -// mode on a live handle, and the archive tier gets its settings from -// the same code path but nothing checked the running file. -func TestArchiveHandleIsWAL(t *testing.T) { - t.Parallel() - - w := delivery.NewExportArchiveWriter( - filepath.Join(t.TempDir(), "archive-wal.db"), - archiveTestLogger(), 0, - ) - - require.NoError(t, w.Open(0)) - - var mode string - - row := w.DB().Raw("pragma journal_mode").Row() - require.NoError(t, row.Scan(&mode)) - assert.Equal(t, "wal", strings.ToLower(mode)) - - var busy string - - row = w.DB().Raw("pragma busy_timeout").Row() - require.NoError(t, row.Scan(&busy)) - assert.Equal(t, "10000", busy) -} - // TestSweepLeavesAQueuedDeliveryAlone is the case the age bound cannot // see. The delivery is queued and untouched, so its row is arbitrarily // old and still perfectly healthy; only ownership distinguishes it diff --git a/internal/delivery/target.go b/internal/delivery/target.go index 157565a..aa819db 100644 --- a/internal/delivery/target.go +++ b/internal/delivery/target.go @@ -103,16 +103,13 @@ func (e *Engine) initTargets(client *http.Client) { client: client, } - dbT := &databaseTarget{eng: e} - e.httpTarget = httpT e.slackTarget = slackT - e.dbTarget = dbT e.targets = map[database.TargetType]Target{ database.TargetTypeHTTP: httpT, database.TargetTypeSlack: slackT, - database.TargetTypeDatabase: dbT, + database.TargetTypeDatabase: &databaseTarget{eng: e}, database.TargetTypeLog: &logTarget{eng: e}, } } diff --git a/internal/delivery/target_config_edit.go b/internal/delivery/target_config_edit.go index 3e76a48..73cd8b2 100644 --- a/internal/delivery/target_config_edit.go +++ b/internal/delivery/target_config_edit.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" + "sneak.berlin/go/webhooker/internal/archive" "sneak.berlin/go/webhooker/internal/database" ) @@ -105,7 +106,7 @@ func databaseConfigForm( return TargetConfigForm{}, nil } - var cfg databaseTargetConfig + var cfg archive.DatabaseTargetConfig err := json.Unmarshal([]byte(configJSON), &cfg) if err != nil { @@ -114,18 +115,18 @@ func databaseConfigForm( ) } - err = ValidateArchiveRotation(cfg.Rotation) + err = archive.ValidateRotation(cfg.Rotation) if err != nil { return TargetConfigForm{}, err } form := TargetConfigForm{Rotation: cfg.Rotation} - if cfg.Expiry == "" || cfg.Expiry == archiveExpiryNever { + if cfg.Expiry == "" || cfg.Expiry == archive.ExpiryNever { return form, nil } - err = ValidateArchiveExpiry(cfg.Expiry) + err = archive.ValidateExpiry(cfg.Expiry) if err != nil { return TargetConfigForm{}, err } diff --git a/internal/delivery/target_config_view.go b/internal/delivery/target_config_view.go index e8b4771..2907b61 100644 --- a/internal/delivery/target_config_view.go +++ b/internal/delivery/target_config_view.go @@ -5,6 +5,7 @@ import ( "strconv" "time" + "sneak.berlin/go/webhooker/internal/archive" "sneak.berlin/go/webhooker/internal/database" ) @@ -200,17 +201,17 @@ func maxAttemptsField(t *database.Target) ConfigField { // not one of the four, is reported as unavailable rather than // echoed back. func databaseConfigFields(configJSON string) []ConfigField { - expiry, err := parseArchiveExpiry(configJSON) + expiry, err := archive.ParseExpiry(configJSON) if err != nil { return unavailableConfigFields() } - rotation, err := parseArchiveRotation(configJSON) + rotation, err := archive.ParseRotation(configJSON) if err != nil { return unavailableConfigFields() } - value := archiveExpiryNever + value := archive.ExpiryNever if expiry > 0 { value = plainDuration(expiry) } diff --git a/internal/delivery/target_config_view_test.go b/internal/delivery/target_config_view_test.go index 8a60b24..595607e 100644 --- a/internal/delivery/target_config_view_test.go +++ b/internal/delivery/target_config_view_test.go @@ -35,6 +35,17 @@ const ( viewMaxAttempts = "Delivery attempts" ) +// The archive rotations a database target's settings can name, and +// the config of one whose rotation is not one of the four. +const ( + rotationNone = "none" + rotationMonthly = "monthly" + rotationDaily = "daily" + rotationHourly = "hourly" + + weeklyConfig = `{"rotation":"weekly"}` +) + func TestMaskedWebhookURL(t *testing.T) { t.Parallel() diff --git a/internal/delivery/target_database.go b/internal/delivery/target_database.go index aead474..9b03171 100644 --- a/internal/delivery/target_database.go +++ b/internal/delivery/target_database.go @@ -2,83 +2,23 @@ package delivery import ( "context" - "fmt" - "strings" - "sync" "time" "gorm.io/gorm" "sneak.berlin/go/webhooker/internal/database" ) -// archiveNameMaxLen is how many characters of a webhook or target -// name an archive file name keeps. -const archiveNameMaxLen = 40 - // databaseTarget is a no-retry target that archives the full // inbound event into the target's own archive SQLite file, separate // from the per-webhook event database. The event is already // persisted in the per-webhook event DB by the time delivery runs; // the database target additionally writes a durable long-term copy -// into the file ArchiveFileName names, with a period added when the -// target rotates (see archivePeriodPath), and then records a single +// through the engine's archive.Writers, and then records a single // attempt whose outcome reflects whether the archive write -// succeeded. See archiveWriter for the close/reopen, auto-recreate, -// and expiry semantics. +// succeeded. Package archive holds everything about the files +// themselves. type databaseTarget struct { eng *Engine - - // writers holds one archive writer per database target, keyed - // by target ID. - mu sync.Mutex - writers map[string]*archiveWriter -} - -// ArchiveFileName returns the file name of a database target's -// archive: archive-WEBHOOKNAME-TARGETNAME-TARGETID.db, with both -// names passed through archiveNamePart. The target ID keeps the -// name unique when two targets' names come out the same. -func ArchiveFileName(webhookName, targetName, targetID string) string { - return "archive-" + archiveNamePart(webhookName) + "-" + - archiveNamePart(targetName) + "-" + targetID + ".db" -} - -// archiveNamePart makes a webhook or target name safe to put in a -// file name. It is lowercased; ASCII letters and digits are kept, -// every other run of characters becomes a single "-", and no "-" is -// left at either end. It is cut to archiveNameMaxLen characters, and -// a name with nothing left is "unnamed". -func archiveNamePart(name string) string { - var b strings.Builder - - dash := false - - for _, r := range strings.ToLower(name) { - if (r < 'a' || r > 'z') && (r < '0' || r > '9') { - dash = b.Len() > 0 - - continue - } - - if dash { - b.WriteByte('-') - - dash = false - } - - b.WriteRune(r) - } - - part := b.String() - if len(part) > archiveNameMaxLen { - part = strings.TrimRight(part[:archiveNameMaxLen], "-") - } - - if part == "" { - return "unnamed" - } - - return part } // Deliver implements Target. It archives the event, then @@ -98,7 +38,7 @@ func (t *databaseTarget) Deliver( ) { start := time.Now() - err := t.archive(d) + err := t.eng.archives.Write(d) elapsed := time.Since(start) @@ -145,308 +85,3 @@ func (t *databaseTarget) Deliver( database.DeliveryStatusDelivered, ) } - -// archive writes the full event as a row into the target's -// archive database, honouring the optional per-target expiry and -// rotation parsed from the target config JSON. With rotation, the -// event goes to the file for the period of its receive time. -func (t *databaseTarget) archive(d *database.Delivery) error { - webhookID := d.Event.WebhookID - if webhookID == "" { - return errArchiveMissingWebhookID - } - - expiry, err := parseArchiveExpiry(d.Target.Config) - if err != nil { - return err - } - - rotation, err := parseArchiveRotation(d.Target.Config) - if err != nil { - return err - } - - // An event whose stored row was gone before its delivery ran has - // no receive time (see Engine.hydrateEvent), and goes to the - // file for now. - receivedAt := d.Event.CreatedAt - if receivedAt.IsZero() { - receivedAt = time.Now() - } - - w, err := t.writerFor(d.TargetID) - if err != nil { - return err - } - - row := archivedEvent{ - EventID: d.Event.ID, - WebhookID: webhookID, - EntrypointID: d.Event.EntrypointID, - Method: d.Event.Method, - RawQuery: d.Event.RawQuery, - Headers: d.Event.Headers, - Body: d.Event.Body, - ContentType: d.Event.ContentType, - } - - return w.write(row, expiry, archivePeriod(rotation, receivedAt)) -} - -// writerFor returns the archive writer for a database target, -// creating and caching it on first use. Each target has one writer -// so its close/reopen debounce state is shared across concurrent -// deliveries, and so a rename and the idle sweep take the same lock -// as its writes. -func (t *databaseTarget) writerFor( - targetID string, -) (*archiveWriter, error) { - t.mu.Lock() - defer t.mu.Unlock() - - w, ok := t.writers[targetID] - if !ok { - var err error - - w, err = t.newWriter(targetID) - if err != nil { - return nil, err - } - - if t.writers == nil { - t.writers = make(map[string]*archiveWriter) - } - - t.writers[targetID] = w - } - - // A delivery claims the entry: even if the idle sweep created - // it moments ago, it now belongs to the registry proper and - // the sweep must leave it in place when it finishes. - w.sweepOwned = false - - return w, nil -} - -// sweepWriterFor returns the archive writer the idle sweep should -// prune a target's archive through, together with whether the sweep -// itself created the registry entry. -// -// The sweep must route its prune through the registered writer so -// the writer's mutex orders it against concurrent writes, but it -// must never leave a registry entry behind: a sweep that ran -// concurrently with the target's deletion would otherwise -// re-create an entry that nothing will ever evict again, which is -// exactly the leak eviction exists to prevent. An entry the sweep -// creates is therefore marked sweep-owned and handed back to -// releaseSweepWriter when the sweep is done. -func (t *databaseTarget) sweepWriterFor( - targetID string, -) (*archiveWriter, bool, error) { - t.mu.Lock() - defer t.mu.Unlock() - - w, ok := t.writers[targetID] - if ok { - return w, false, nil - } - - w, err := t.newWriter(targetID) - if err != nil { - return nil, false, err - } - - if t.writers == nil { - t.writers = make(map[string]*archiveWriter) - } - - w.sweepOwned = true - t.writers[targetID] = w - - return w, true, nil -} - -// releaseSweepWriter drops a registry entry that the idle sweep -// created, so a sweep leaves the registry exactly as it found it. -// -// The entry is removed only if it is still the very writer the -// sweep installed and no delivery has claimed it in the meantime -// (writerFor clears sweepOwned when it hands a writer to the -// write path). Both conditions are evaluated under the registry -// lock, so an eviction that raced the sweep — which removes the -// entry outright — simply finds nothing left to do here, and a -// delivery that adopted the writer keeps a registered, evictable -// one. -func (t *databaseTarget) releaseSweepWriter( - targetID string, w *archiveWriter, -) { - t.mu.Lock() - defer t.mu.Unlock() - - cur, ok := t.writers[targetID] - if !ok || cur != w || !cur.sweepOwned { - return - } - - delete(t.writers, targetID) -} - -// newWriter builds the writer for a database target's archive. Its -// path is the one ArchivePath gives for the webhook and the target as -// the main database names them now; from then on only rename changes -// the name the writer uses. It does not touch the archive files. -func (t *databaseTarget) newWriter( - targetID string, -) (*archiveWriter, error) { - if t.eng.dbManager == nil { - return nil, errArchiveNoDataDir - } - - var target database.Target - - err := t.eng.database.DB(). - Preload("Webhook"). - First(&target, "id = ?", targetID).Error - if err != nil { - return nil, fmt.Errorf( - "loading database target %s: %w", targetID, err, - ) - } - - w := newArchiveWriter( - ArchivePath(t.eng.dbManager, &target.Webhook, &target), - t.eng.log, - ) - w.webhookID = target.WebhookID - - return w, nil -} - -// rename moves every one of a database target's archive files to the -// name for webhookName and targetName. It goes through the target's -// writer, so the move holds the lock that writes and the idle sweep -// take, and later writes use the new name. -// -// The writer is created if there is none, and it stays cached. The -// handlers rename before they save the new name, so until the save -// the main database still has the old one; a delivery in that window -// must find this writer rather than build one from the old name. -func (t *databaseTarget) rename( - targetID, webhookName, targetName string, -) error { - w, err := t.writerFor(targetID) - if err != nil { - return err - } - - return w.rename(ArchiveFileName(webhookName, targetName, targetID)) -} - -// evict drops a database target's archive writer from the registry -// and closes its handle, so a deleted target does not leave a -// writer (and an open archive handle within its debounce window) -// alive for the process lifetime. -// -// The map entry is removed under the registry lock, which is -// then released before the handle is closed under the writer's -// own lock: that ordering keeps the registry available to other -// targets while an in-flight write on this one drains, and -// closing under the writer's lock means eviction can never race -// a write. -// -// Eviction is idempotent and silent for a target with no writer, -// which is the common case: only a database target that has -// received an event or been renamed has one. It never deletes the -// archive file. -func (t *databaseTarget) evict(targetID string) { - t.mu.Lock() - - w, ok := t.writers[targetID] - if ok { - delete(t.writers, targetID) - } - - t.mu.Unlock() - - if !ok { - return - } - - w.evict() - - t.eng.log.Info( - "evicted archive writer", - "target_id", targetID, - "path", w.path, - ) -} - -// evictWebhook evicts, exactly as evict does, the writer of every -// database target of a webhook. -func (t *databaseTarget) evictWebhook(webhookID string) { - t.mu.Lock() - - var gone []*archiveWriter - - for targetID, w := range t.writers { - if w.webhookID == webhookID { - delete(t.writers, targetID) - - gone = append(gone, w) - } - } - - t.mu.Unlock() - - for _, w := range gone { - w.evict() - - t.eng.log.Info( - "evicted archive writer", - "webhook_id", webhookID, - "path", w.path, - ) - } -} - -// evictAll evicts every cached archive writer, exactly as evict -// does for one target. The engine calls it at shutdown, once its -// workers have returned. Closing the last handle on an archive -// moves the contents of its -wal into the .db and removes the -// -wal, so a clean stop leaves each archive as a single file. -func (t *databaseTarget) evictAll() { - t.mu.Lock() - - writers := t.writers - t.writers = nil - - t.mu.Unlock() - - for _, w := range writers { - w.evict() - } -} - -// sweepArchive prunes one database target's archive of rows older -// than expiry, without requiring a write. A missing archive file is -// left missing (see sweepExpired), so a sweep never creates an -// archive for a target that has never received an event. -// -// It also never leaves a registry entry behind: an entry it had -// to create to reach the writer's mutex is released again once -// the prune is done, so a sweep racing a target deletion cannot -// resurrect the writer the eviction just dropped. -func (t *databaseTarget) sweepArchive( - targetID string, expiry time.Duration, -) error { - w, created, err := t.sweepWriterFor(targetID) - if err != nil { - return err - } - - if created { - defer t.releaseSweepWriter(targetID, w) - } - - return w.sweepExpired(expiry) -} diff --git a/internal/delivery/target_database_test.go b/internal/delivery/target_database_test.go index 7c96842..48b20c6 100644 --- a/internal/delivery/target_database_test.go +++ b/internal/delivery/target_database_test.go @@ -3,24 +3,65 @@ package delivery_test import ( "database/sql" "fmt" - "io/fs" "log/slog" + "net/http" "os" "path/filepath" - "strings" "testing" "time" + "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gorm.io/driver/sqlite" "gorm.io/gorm" + "gorm.io/gorm/clause" _ "modernc.org/sqlite" // Pure Go SQLite driver. + "sneak.berlin/go/webhooker/internal/archive" "sneak.berlin/go/webhooker/internal/database" + "sneak.berlin/go/webhooker/internal/database/databasetest" "sneak.berlin/go/webhooker/internal/delivery" "sneak.berlin/go/webhooker/internal/gormlog" ) +// archiveTestWebhookName is the name of every webhook +// seedDatabaseTarget creates. It is not safe in a file name as it +// stands, so every archive file name made from it is made safe first. +const archiveTestWebhookName = "Sweep Test!" + +// archiveEnv bundles the pieces a database target test drives: a main +// configuration database holding webhooks and targets, a delivery +// engine whose archive writers write into the data directory, and that +// directory. +type archiveEnv struct { + eng *delivery.Engine + mainDB *database.Database + dataDir string +} + +func setupArchiveTest(t *testing.T) *archiveEnv { + t.Helper() + + dataDir := t.TempDir() + + mainDB, err := database.Open(dataDir, slog.New(slog.DiscardHandler)) + require.NoError(t, err) + + t.Cleanup(func() { _ = mainDB.Close() }) + + return &archiveEnv{ + eng: delivery.NewTestEngineWithDB( + mainDB, + databasetest.NewWebhookDBManager(t, dataDir), + archiveTestLogger(), + &http.Client{Timeout: 5 * time.Second}, + 1, + ), + mainDB: mainDB, + dataDir: dataDir, + } +} + func archiveTestLogger() *slog.Logger { return slog.New(slog.NewTextHandler( os.Stderr, @@ -28,8 +69,85 @@ func archiveTestLogger() *slog.Logger { )) } +// archivePath returns where the engine keeps a database target's +// archive file, for the names seedDatabaseTarget gave it. +func (env *archiveEnv) archivePath(tgt *database.Target) string { + return filepath.Join( + env.dataDir, + archive.FileName(archiveTestWebhookName, tgt.Name, tgt.ID), + ) +} + +// seedDatabaseTarget creates a webhook with one database target +// carrying the given target config JSON, and returns the target. +func (env *archiveEnv) seedDatabaseTarget( + t *testing.T, configJSON string, +) *database.Target { + t.Helper() + + wh := &database.Webhook{ + UserID: uuid.New().String(), + Name: archiveTestWebhookName, + } + require.NoError( + t, + env.mainDB.DB(). + Omit(clause.Associations). + Create(wh).Error, + ) + + tgt := &database.Target{ + WebhookID: wh.ID, + Name: "Archive", + Type: database.TargetTypeDatabase, + Active: true, + Config: configJSON, + MaxAttempts: 1, + } + require.NoError( + t, + env.mainDB.DB(). + Omit(clause.Associations). + Create(tgt).Error, + ) + + return tgt +} + +// seedDatabaseTargetDelivery seeds a pending delivery of an event +// to a database target and returns the in-memory delivery the +// target handler is invoked with. +func seedDatabaseTargetDelivery( + t *testing.T, + webhookDB *gorm.DB, + event database.Event, + tgt *database.Target, +) *database.Delivery { + t.Helper() + + dlv := seedDelivery( + t, webhookDB, event.ID, tgt.ID, + database.DeliveryStatusPending, + ) + + d := &database.Delivery{ + EventID: event.ID, + TargetID: tgt.ID, + Status: database.DeliveryStatusPending, + Event: event, + Target: database.Target{ + Name: tgt.Name, + Type: database.TargetTypeDatabase, + Config: tgt.Config, + }, + } + d.ID = dlv.ID + + return d +} + // openArchiveDBForRead opens an archive file read-only so a -// test can inspect the rows the writer persisted. +// test can inspect the rows the database target archived. func openArchiveDBForRead( t *testing.T, path string, ) *gorm.DB { @@ -52,26 +170,45 @@ func openArchiveDBForRead( return gdb } -// archiveFileSuffixes returns the archive file itself and the -// SQLite sidecars that accompany an open database. A test that -// asserts no archive was created has to check all of them. -func archiveFileSuffixes() []string { - return []string{"", "-wal", "-shm"} +// countArchivedRows counts the rows in an archive file without +// asserting anything, so it is safe to poll from an +// assert.Eventually condition (which runs off the test +// goroutine, where testify assertions must not be used). +func countArchivedRows(path string) (int64, error) { + sqlDB, err := sql.Open( + "sqlite", fmt.Sprintf("file:%s?mode=ro", path), + ) + if err != nil { + return 0, err + } + + defer func() { _ = sqlDB.Close() }() + + gdb, err := gorm.Open( + sqlite.Dialector{Conn: sqlDB}, + &gorm.Config{Logger: gormlog.New(slog.New(slog.DiscardHandler))}, + ) + if err != nil { + return 0, err + } + + var count int64 + + err = gdb.Table("archived_events").Count(&count).Error + if err != nil { + return 0, err + } + + return count, nil } -// removeArchiveFiles simulates an operator moving the archive -// away by deleting the SQLite file and its sidecar files. -func removeArchiveFiles(t *testing.T, path string) { - t.Helper() - - for _, suffix := range []string{ - "", "-wal", "-shm", "-journal", - } { - err := os.Remove(path + suffix) - if err != nil && !os.IsNotExist(err) { - t.Fatalf("removing %s%s: %v", path, suffix, err) - } - } +// archivedRow is the columns of an archive row these tests read. +type archivedRow struct { + EventID string + WebhookID string + Method string + RawQuery string + Body string } // TestDeliverDatabase_ArchivesEvent verifies that delivering to @@ -107,9 +244,9 @@ func TestDeliverDatabase_ArchivesEvent(t *testing.T) { rdb := openArchiveDBForRead(t, archivePath) - var rows []delivery.ExportArchivedEvent + var rows []archivedRow - require.NoError(t, rdb.Find(&rows).Error) + require.NoError(t, rdb.Table("archived_events").Find(&rows).Error) require.Len(t, rows, 1) assert.Equal(t, event.ID, rows[0].EventID) assert.Equal(t, event.WebhookID, rows[0].WebhookID) @@ -118,262 +255,6 @@ func TestDeliverDatabase_ArchivesEvent(t *testing.T) { assert.JSONEq(t, `{"archived":true}`, rows[0].Body) } -func TestArchiveWriter_WritesRow(t *testing.T) { - t.Parallel() - - path := filepath.Join(t.TempDir(), "archive-wh.db") - w := delivery.NewExportArchiveWriter( - path, archiveTestLogger(), 0, - ) - - row := delivery.ExportArchivedEvent{ - EventID: "ev-1", - WebhookID: "wh-1", - EntrypointID: "ep-1", - Method: "POST", - Headers: `{"X":"Y"}`, - Body: `{"hello":"world"}`, - ContentType: "application/json", - } - - require.NoError(t, w.Write(row, 0)) - assert.FileExists(t, path) - - var got []delivery.ExportArchivedEvent - - require.NoError(t, w.DB().Find(&got).Error) - require.Len(t, got, 1) - assert.Equal(t, "ev-1", got[0].EventID) - assert.Equal(t, "wh-1", got[0].WebhookID) - assert.Equal(t, "ep-1", got[0].EntrypointID) - assert.Equal(t, row.Method, got[0].Method) - assert.Equal(t, row.ContentType, got[0].ContentType) - assert.JSONEq(t, `{"hello":"world"}`, got[0].Body) - assert.False(t, got[0].ArchivedAt.IsZero()) - - var archivedAt string - - require.NoError(t, w.DB().Raw( - "SELECT CAST(archived_at AS TEXT) FROM archived_events", - ).Row().Scan(&archivedAt)) - assertStoredTime(t, archivedAt) -} - -func TestArchiveWriter_RecreatesAfterRemoval( - t *testing.T, -) { - t.Parallel() - - path := filepath.Join(t.TempDir(), "archive-wh.db") - w := delivery.NewExportArchiveWriter( - path, archiveTestLogger(), 0, - ) - - require.NoError(t, w.Write( - delivery.ExportArchivedEvent{EventID: "a"}, 0, - )) - assert.FileExists(t, path) - - // The operator moves the archive away while the handle is - // still open. - removeArchiveFiles(t, path) - require.NoFileExists(t, path) - - // The next write recreates the file with a fresh schema and - // only the new row. - require.NoError(t, w.Write( - delivery.ExportArchivedEvent{EventID: "b"}, 0, - )) - assert.FileExists(t, path) - - var got []delivery.ExportArchivedEvent - - require.NoError(t, w.DB().Find(&got).Error) - require.Len(t, got, 1) - assert.Equal(t, "b", got[0].EventID) -} - -// TestStatArchive proves StatArchive finds no file before the first -// write; after a write still held in the -wal, counts the -wal in the -// size and takes its later time as the last write; and finds no file -// again once the file has been moved away. -func TestStatArchive(t *testing.T) { - t.Parallel() - - path := filepath.Join(t.TempDir(), "archive-wh.db") - - _, err := delivery.StatArchive(path) - require.ErrorIs(t, err, fs.ErrNotExist) - - // With the clock stopped, the reopen debounce never passes, so - // the handle stays open after the write. - stopped := time.Now() - w := delivery.NewExportArchiveWriter(path, archiveTestLogger(), 0) - w.SetNow(func() time.Time { return stopped }) - - require.NoError(t, w.Write(delivery.ExportArchivedEvent{EventID: "a"}, 0)) - - written := time.Date(2026, 1, 2, 3, 4, 5, 0, time.UTC) - earlier := written.Add(-time.Hour) - require.NoError(t, os.Chtimes(path, earlier, earlier)) - require.NoError(t, os.Chtimes(path+"-wal", written, written)) - - file, err := os.Stat(path) - require.NoError(t, err) - - wal, err := os.Stat(path + "-wal") - require.NoError(t, err) - require.Positive(t, wal.Size()) - - got, err := delivery.StatArchive(path) - require.NoError(t, err) - assert.Equal(t, 1, got.Files) - assert.Equal(t, file.Size()+wal.Size(), got.Size) - assert.True(t, written.Equal(got.Written), got.Written) - - removeArchiveFiles(t, path) - - _, err = delivery.StatArchive(path) - require.ErrorIs(t, err, fs.ErrNotExist) -} - -func TestArchiveWriter_ReopenDebounce(t *testing.T) { - t.Parallel() - - const debounce = 2 * time.Second - - path := filepath.Join(t.TempDir(), "archive-wh.db") - w := delivery.NewExportArchiveWriter( - path, archiveTestLogger(), debounce, - ) - - // The writer measures its reopen debounce on this clock, which - // only the test moves, so how long the host takes between - // writes cannot change the result. - now := time.Now() - - w.SetNow(func() time.Time { return now }) - - require.NoError(t, w.Write( - delivery.ExportArchivedEvent{EventID: "a"}, 0, - )) - require.NoError(t, w.Write( - delivery.ExportArchivedEvent{EventID: "b"}, 0, - )) - - // Two writes inside the debounce window trigger only the - // initial open — no extra close/reopen. - assert.Equal(t, 1, w.Reopens()) - - now = now.Add(debounce) - - require.NoError(t, w.Write( - delivery.ExportArchivedEvent{EventID: "c"}, 0, - )) - - // A write after the window elapses closes and reopens once. - assert.Equal(t, 2, w.Reopens()) -} - -func TestArchiveWriter_ExpiryPrune(t *testing.T) { - t.Parallel() - - path := filepath.Join(t.TempDir(), "archive-wh.db") - w := delivery.NewExportArchiveWriter( - path, archiveTestLogger(), 0, - ) - - require.NoError(t, w.Open(0)) - - old := delivery.ExportArchivedEvent{ - EventID: "old", - ArchivedAt: time.Now().Add(-2 * time.Hour), - } - fresh := delivery.ExportArchivedEvent{ - EventID: "fresh", - ArchivedAt: time.Now(), - } - - require.NoError(t, w.DB().Create(&old).Error) - require.NoError(t, w.DB().Create(&fresh).Error) - - // Reopening with a one-hour expiry prunes the old row. - require.NoError(t, w.Reopen(time.Hour)) - - var got []delivery.ExportArchivedEvent - - require.NoError(t, w.DB().Find(&got).Error) - require.Len(t, got, 1) - assert.Equal(t, "fresh", got[0].EventID) -} - -func TestParseArchiveExpiry(t *testing.T) { - t.Parallel() - - cases := []struct { - name string - in string - want time.Duration - wantErr bool - }{ - {"empty config", "", 0, false}, - {"explicit never", `{"expiry":"never"}`, 0, false}, - {"empty expiry", `{"expiry":""}`, 0, false}, - {"duration", `{"expiry":"1h"}`, time.Hour, false}, - {"unparseable", `{"expiry":"nonsense"}`, 0, true}, - {"zero duration", `{"expiry":"0s"}`, 0, true}, - {"negative duration", `{"expiry":"-5h"}`, 0, true}, - } - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - - got, err := delivery.ExportParseArchiveExpiry(tc.in) - if tc.wantErr { - require.Error(t, err) - - return - } - - require.NoError(t, err) - assert.Equal(t, tc.want, got) - }) - } -} - -// seedDatabaseTargetDelivery seeds a pending delivery of an event -// to a database target and returns the in-memory delivery the -// target handler is invoked with. -func seedDatabaseTargetDelivery( - t *testing.T, - webhookDB *gorm.DB, - event database.Event, - tgt *database.Target, -) *database.Delivery { - t.Helper() - - dlv := seedDelivery( - t, webhookDB, event.ID, tgt.ID, - database.DeliveryStatusPending, - ) - - d := &database.Delivery{ - EventID: event.ID, - TargetID: tgt.ID, - Status: database.DeliveryStatusPending, - Event: event, - Target: database.Target{ - Name: tgt.Name, - Type: database.TargetTypeDatabase, - Config: tgt.Config, - }, - } - d.ID = dlv.ID - - return d -} - // TestDeliverDatabase_ArchiveFailureFailsDelivery verifies that // an archive error (here: an unparseable expiry in the target // config) fails the delivery loudly: the attempt is recorded as @@ -423,348 +304,3 @@ func TestDeliverDatabase_ArchiveFailureFailsDelivery( "no archive file should exist for a failed config", ) } - -func TestValidateArchiveExpiry(t *testing.T) { - t.Parallel() - - valid := []string{"", "never", "1h", "720h", "30m"} - for _, in := range valid { - require.NoError(t, - delivery.ValidateArchiveExpiry(in), - "expiry %q should be accepted", in, - ) - } - - invalid := []string{"nonsense", "7d", "-5h", "0s", "0"} - for _, in := range invalid { - require.Error(t, - delivery.ValidateArchiveExpiry(in), - "expiry %q should be rejected", in, - ) - } -} - -// TestArchiveFileName pins the archive file name and the rules -// that make a webhook or target name safe to put in it. -func TestArchiveFileName(t *testing.T) { - t.Parallel() - - const id = "3f2a1c9e-8d4b-4c1a-9e2f-0a1b2c3d4e5f" - - cases := []struct { - name string - webhook string - target string - want string - }{ - { - "plain names", "orders", "archive", - "archive-orders-archive-" + id + ".db", - }, - { - "lowercased", "Orders", "Main Archive", - "archive-orders-main-archive-" + id + ".db", - }, - { - "a run of other characters is one dash", - `a /\..b`, "c__--d", - "archive-a-b-c-d-" + id + ".db", - }, - { - "no dash at either end", " --orders!! ", "(archive)", - "archive-orders-archive-" + id + ".db", - }, - { - "path separators", "../../etc/passwd", "a/b", - "archive-etc-passwd-a-b-" + id + ".db", - }, - { - "letters outside ASCII are dropped", - "Bestellungen Größe", "café", - "archive-bestellungen-gr-e-caf-" + id + ".db", - }, - { - "nothing left is unnamed", "", "!!!", - "archive-unnamed-unnamed-" + id + ".db", - }, - { - "cut to 40 characters", strings.Repeat("a", 50), "x", - "archive-" + strings.Repeat("a", 40) + "-x-" + id + ".db", - }, - { - "no dash left by the cut", - strings.Repeat("a", 39) + " b", "x", - "archive-" + strings.Repeat("a", 39) + "-x-" + id + ".db", - }, - } - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - - assert.Equal( - t, tc.want, - delivery.ArchiveFileName(tc.webhook, tc.target, id), - ) - }) - } -} - -// TestDeliverDatabase_EachTargetHasItsOwnArchive proves two -// database targets of one webhook archive into separate files. -func TestDeliverDatabase_EachTargetHasItsOwnArchive(t *testing.T) { - t.Parallel() - - env := setupArchiveTest(t) - first := env.seedDatabaseTarget(t, "") - second := env.addDatabaseTarget(t, first.WebhookID, "") - - webhookDB := testWebhookDB(t) - event := seedEvent(t, webhookDB, `{"n":1}`) - - for _, tgt := range []*database.Target{first, second} { - env.eng.ExportDeliverDatabase( - webhookDB, - seedDatabaseTargetDelivery(t, webhookDB, event, tgt), - ) - } - - require.NotEqual( - t, env.archivePath(first), env.archivePath(second), - ) - assert.Equal( - t, []string{event.ID}, - archivedEventIDs(t, env.archivePath(first)), - ) - assert.Equal( - t, []string{event.ID}, - archivedEventIDs(t, env.archivePath(second)), - ) -} - -// TestRename_MovesTheFile proves a rename moves the archive, rows -// and all, and that later writes go to the new name. -func TestRename_MovesTheFile(t *testing.T) { - t.Parallel() - - env := setupArchiveTest(t) - tgt := env.seedDatabaseTarget(t, "") - oldPath := env.archivePath(tgt) - - webhookDB := testWebhookDB(t) - first := seedEvent(t, webhookDB, `{"n":1}`) - env.eng.ExportDeliverDatabase( - webhookDB, seedDatabaseTargetDelivery(t, webhookDB, first, tgt), - ) - require.FileExists(t, oldPath) - - require.NoError( - t, env.eng.Rename(tgt.ID, "Orders", "Long Term"), - ) - - newPath := filepath.Join( - env.dataDir, "archive-orders-long-term-"+tgt.ID+".db", - ) - - assert.NoFileExists(t, oldPath) - assert.Equal(t, []string{first.ID}, archivedEventIDs(t, newPath)) - - second := seedEvent(t, webhookDB, `{"n":2}`) - env.eng.ExportDeliverDatabase( - webhookDB, - seedDatabaseTargetDelivery(t, webhookDB, second, tgt), - ) - - assert.ElementsMatch( - t, []string{first.ID, second.ID}, - archivedEventIDs(t, newPath), - ) - assert.NoFileExists( - t, oldPath, "a write after the rename must use the new name", - ) -} - -// TestRename_NeverReplacesAFile plants a file at the new name, once -// the .db alone, once a lone -wal and once a lone -shm, and proves -// each time that the rename is refused, the planted file survives, -// and the archive keeps its name and its rows. -func TestRename_NeverReplacesAFile(t *testing.T) { - t.Parallel() - - for _, suffix := range archiveFileSuffixes() { - t.Run("planted .db"+suffix, func(t *testing.T) { - t.Parallel() - - env := setupArchiveTest(t) - tgt := env.seedDatabaseTarget(t, "") - oldPath := env.archivePath(tgt) - - webhookDB := testWebhookDB(t) - first := seedEvent(t, webhookDB, `{"n":1}`) - env.eng.ExportDeliverDatabase( - webhookDB, - seedDatabaseTargetDelivery(t, webhookDB, first, tgt), - ) - - newPath := filepath.Join( - env.dataDir, "archive-orders-long-term-"+tgt.ID+".db", - ) - plantedPath := newPath + suffix - require.NoError( - t, os.WriteFile(plantedPath, []byte("planted"), 0o600), - ) - - require.ErrorIs( - t, env.eng.Rename(tgt.ID, "Orders", "Long Term"), - delivery.ErrArchiveNameTaken, - ) - - //nolint:gosec // reads the file the test planted under t.TempDir() - planted, err := os.ReadFile(plantedPath) - require.NoError(t, err) - assert.Equal(t, "planted", string(planted)) - - second := seedEvent(t, webhookDB, `{"n":2}`) - env.eng.ExportDeliverDatabase( - webhookDB, - seedDatabaseTargetDelivery(t, webhookDB, second, tgt), - ) - - assert.ElementsMatch( - t, []string{first.ID, second.ID}, - archivedEventIDs(t, oldPath), - ) - }) - } -} - -// TestRename_BeforeTheNameIsSaved covers the order the handlers -// use: they rename before they save the new name, so a delivery in -// between must write under the new name although the main database -// still has the old one. It also shows that renaming an archive that -// does not exist yet is not an error. -func TestRename_BeforeTheNameIsSaved(t *testing.T) { - t.Parallel() - - env := setupArchiveTest(t) - tgt := env.seedDatabaseTarget(t, "") - - require.NoError( - t, env.eng.Rename(tgt.ID, "Orders", "Archive"), - ) - - webhookDB := testWebhookDB(t) - event := seedEvent(t, webhookDB, `{"n":1}`) - env.eng.ExportDeliverDatabase( - webhookDB, seedDatabaseTargetDelivery(t, webhookDB, event, tgt), - ) - - assert.FileExists( - t, - filepath.Join( - env.dataDir, "archive-orders-archive-"+tgt.ID+".db", - ), - ) - assert.NoFileExists(t, env.archivePath(tgt)) -} - -// TestArchiveWriter_RenameMovesSidecars proves a rename carries -// the -wal and -shm a crash can leave beside an archive no handle -// has opened since. SQLite finds them by name, so a -wal left -// behind would lose the transactions it holds. -func TestArchiveWriter_RenameMovesSidecars(t *testing.T) { - t.Parallel() - - dir := t.TempDir() - oldPath := filepath.Join(dir, "archive-old.db") - newPath := filepath.Join(dir, "archive-new.db") - - for _, suffix := range archiveFileSuffixes() { - require.NoError( - t, os.WriteFile(oldPath+suffix, []byte(suffix), 0o600), - ) - } - - w := delivery.NewExportArchiveWriter( - oldPath, archiveTestLogger(), 0, - ) - - require.NoError(t, w.Rename("archive-new.db")) - - for _, suffix := range archiveFileSuffixes() { - assert.NoFileExists(t, oldPath+suffix) - assert.FileExists(t, newPath+suffix) - } - - assert.Equal(t, newPath, w.Path()) -} - -// TestArchiveWriter_RenameMovesBackOnFailure makes the -wal fail to -// move after the .db has moved, and proves the .db is moved back, so -// the archive is never split across two names. The new name is 255 -// bytes, the longest a file name may be, so the .db can take it but -// the -wal, four bytes longer, cannot. -func TestArchiveWriter_RenameMovesBackOnFailure(t *testing.T) { - t.Parallel() - - dir := t.TempDir() - oldPath := filepath.Join(dir, "archive-old.db") - newName := strings.Repeat("a", 252) + ".db" - - for _, suffix := range archiveFileSuffixes() { - require.NoError( - t, os.WriteFile(oldPath+suffix, []byte(suffix), 0o600), - ) - } - - w := delivery.NewExportArchiveWriter( - oldPath, archiveTestLogger(), 0, - ) - - require.Error(t, w.Rename(newName)) - - for _, suffix := range archiveFileSuffixes() { - assert.FileExists(t, oldPath+suffix) - } - - assert.NoFileExists(t, filepath.Join(dir, newName)) - assert.Equal(t, oldPath, w.Path()) -} - -// TestArchiveWriter_RenameMovesBackEveryFile renames a target with a -// file without a period and a file for a month, and proves that when -// the month's file fails to move, the file already moved is moved back: -// both files are under the old name with their rows, and nothing is -// under the new name. The new name is 251 bytes, so the file without a -// period, with its -wal and -shm, can take it, but the month's file, -// eight bytes longer, cannot. -func TestArchiveWriter_RenameMovesBackEveryFile(t *testing.T) { - t.Parallel() - - dir := t.TempDir() - oldPath := filepath.Join(dir, "archive-old.db") - monthPath := filepath.Join(dir, "archive-old-2026-03.db") - w := delivery.NewExportArchiveWriter(oldPath, archiveTestLogger(), 0) - - require.NoError(t, w.WritePeriod( - delivery.ExportArchivedEvent{EventID: "in-none"}, 0, "", - )) - require.NoError(t, w.WritePeriod( - delivery.ExportArchivedEvent{EventID: "in-month"}, 0, "2026-03", - )) - - require.Error(t, w.Rename(strings.Repeat("a", 248)+".db")) - - assert.Equal(t, []string{"in-none"}, archivedEventIDs(t, oldPath)) - assert.Equal(t, []string{"in-month"}, archivedEventIDs(t, monthPath)) - - entries, err := os.ReadDir(dir) - require.NoError(t, err) - - for _, entry := range entries { - assert.True(t, strings.HasPrefix(entry.Name(), "archive-old"), - "%s is not under the old name", entry.Name()) - } - - assert.Equal(t, oldPath, w.Path()) -} diff --git a/internal/handlers/archive_rotation_test.go b/internal/handlers/archive_rotation_test.go index ff2c511..94b27bc 100644 --- a/internal/handlers/archive_rotation_test.go +++ b/internal/handlers/archive_rotation_test.go @@ -12,8 +12,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "sneak.berlin/go/webhooker/internal/archive" "sneak.berlin/go/webhooker/internal/database" - "sneak.berlin/go/webhooker/internal/delivery" ) // rotationNone is the archive rotation that keeps one file. @@ -181,7 +181,7 @@ func TestArchiveFileView_Rotated(t *testing.T) { `{"rotation":"daily"}`, ) - path := delivery.ArchivePath(env.dbMgr, &webhook, target) + path := archive.Path(env.dbMgr, &webhook, target) stem := strings.TrimSuffix(path, ".db") written := time.Date(2026, 10, 2, 9, 0, 0, 0, time.UTC) diff --git a/internal/handlers/gormlogbound_test.go b/internal/handlers/gormlogbound_test.go index d5458dc..21a8743 100644 --- a/internal/handlers/gormlogbound_test.go +++ b/internal/handlers/gormlogbound_test.go @@ -374,7 +374,7 @@ func assertFloodBounded(t *testing.T, label, out string) { // this service's three gorm.Open sites: the main database behind the // two unauthenticated lookups, and the per-webhook database the // WebhookDBManager opens. The third, the archive writer, is pinned in -// internal/delivery, where its type lives. +// internal/archive, where its type lives. // // What each assertion is worth, since two of the three would pass // against a service that had never been fixed if the capture were set diff --git a/internal/handlers/handlers.go b/internal/handlers/handlers.go index 2b689e6..0770809 100644 --- a/internal/handlers/handlers.go +++ b/internal/handlers/handlers.go @@ -10,11 +10,11 @@ import ( "html/template" "log/slog" "net/http" - "sync" "sync/atomic" "github.com/prometheus/client_golang/prometheus" "go.uber.org/fx" + "sneak.berlin/go/webhooker/internal/archive" "sneak.berlin/go/webhooker/internal/config" "sneak.berlin/go/webhooker/internal/database" "sneak.berlin/go/webhooker/internal/delivery" @@ -65,7 +65,7 @@ type HandlersParams struct { Session *session.Session Middleware *middleware.Middleware Notifier delivery.Notifier - Archives delivery.Archives + Archives archive.Archives CircuitBreakers delivery.CircuitBreakers SSRFGuard *delivery.Guard Metrics *metrics.Set @@ -83,7 +83,7 @@ type Handlers struct { session *session.Session mw *middleware.Middleware notifier delivery.Notifier - archives delivery.Archives + archives archive.Archives breakers delivery.CircuitBreakers mtr *metrics.Set templates map[string]*template.Template @@ -93,17 +93,6 @@ type Handlers struct { // is one delivery will actually attempt. ssrf *delivery.Guard - // renameMu makes the webhook edit, the target edit and target - // creation run one at a time, each held from loading the stored - // names through the archive rename, the save and any move back. - // Interleaved, one could rename an archive between another's - // rename and save, leaving the file named for one edit and the - // stored names from the other. An archive download holds it while - // it reads the stored names and lists the files they give, and - // again for each file while it finds the file under the names - // stored then and opens it. - renameMu sync.Mutex - // dummyVerifications counts the equivalent-cost verifications // charged for usernames that do not exist. It exists so a test // can prove that path runs without measuring wall-clock time. diff --git a/internal/handlers/handlers_test.go b/internal/handlers/handlers_test.go index 2cbeaf1..0c48f83 100644 --- a/internal/handlers/handlers_test.go +++ b/internal/handlers/handlers_test.go @@ -16,6 +16,7 @@ import ( "github.com/stretchr/testify/require" "go.uber.org/fx" "go.uber.org/fx/fxtest" + "sneak.berlin/go/webhooker/internal/archive" "sneak.berlin/go/webhooker/internal/config" "sneak.berlin/go/webhooker/internal/database" "sneak.berlin/go/webhooker/internal/delivery" @@ -55,12 +56,14 @@ func (n *recordingNotifier) Tasks() []delivery.Task { return out } -// recordingArchives is a delivery.Archives that records what it +// recordingArchives is an archive.Archives that records what it // was asked to do, so a test can prove that a deletion or rename -// path reached the delivery engine. After FailRenames, every +// path reached the archive writers. After FailRenames, every // rename of that target fails with the given error. After // BlockNextRename, the next rename is recorded and then waits. +// Lock and Unlock are a plain mutex, as in archive.Writers. type recordingArchives struct { + renameMu sync.Mutex mu sync.Mutex evicted []string evictedTargets []string @@ -73,10 +76,10 @@ type recordingArchives struct { // errInjectedRename is the failure a test hands FailRenames. var errInjectedRename = errors.New("injected rename failure") -// errNameTaken is what the delivery engine returns when a file +// errNameTaken is what the archive writers return when a file // already has an archive's new name, here archive-taken.db. var errNameTaken = fmt.Errorf( - "%w: archive-taken.db", delivery.ErrArchiveNameTaken, + "%w: archive-taken.db", archive.ErrNameTaken, ) // archiveRename is one recorded Rename call. @@ -86,6 +89,14 @@ type archiveRename struct { TargetName string } +func (r *recordingArchives) Lock() { + r.renameMu.Lock() +} + +func (r *recordingArchives) Unlock() { + r.renameMu.Unlock() +} + func (r *recordingArchives) EvictWebhook(webhookID string) { r.mu.Lock() defer r.mu.Unlock() @@ -265,7 +276,7 @@ func newTestAppWithConfig( func() *recordingArchives { return &recordingArchives{} }, - func(r *recordingArchives) delivery.Archives { + func(r *recordingArchives) archive.Archives { return r }, func() *testCircuitBreakers { diff --git a/internal/handlers/source_delete_test.go b/internal/handlers/source_delete_test.go index 4ef4143..5d7b8eb 100644 --- a/internal/handlers/source_delete_test.go +++ b/internal/handlers/source_delete_test.go @@ -16,8 +16,8 @@ import ( "github.com/stretchr/testify/require" "gorm.io/gorm" "gorm.io/gorm/clause" + "sneak.berlin/go/webhooker/internal/archive" "sneak.berlin/go/webhooker/internal/database" - "sneak.berlin/go/webhooker/internal/delivery" "sneak.berlin/go/webhooker/internal/handlers" "sneak.berlin/go/webhooker/internal/session" ) @@ -180,7 +180,7 @@ func failSaveOnTable( } // archivePathFor returns the archive database path the -// delivery engine would use for a database target: beside the +// archive writers would use for a database target: beside the // webhook's event database in the data directory. func archivePathFor( t *testing.T, @@ -192,7 +192,7 @@ func archivePathFor( return filepath.Join( filepath.Dir(mgr.DBPath(wh.ID)), - delivery.ArchiveFileName(wh.Name, tgt.Name, tgt.ID), + archive.FileName(wh.Name, tgt.Name, tgt.ID), ) } @@ -228,9 +228,9 @@ func postRequest( } // TestHandleSourceDelete_EvictsArchiveWriter proves that -// deleting a webhook reaches the delivery engine and releases -// the webhook's archive writers, exercised through the real -// deletion handler rather than by calling the engine directly. +// deleting a webhook releases the webhook's archive writers, +// exercised through the real deletion handler rather than by +// calling the archive writers directly. func TestHandleSourceDelete_EvictsArchiveWriter(t *testing.T) { t.Parallel() @@ -290,7 +290,7 @@ func TestHandleSourceDelete_KeepsArchiveFile(t *testing.T) { wh := seedWebhook(t, db) tgt := seedTarget(t, db, wh.ID, database.TargetTypeDatabase) - // Place an archive file where the delivery engine would. + // Place an archive file where the archive writers would. archivePath := archivePathFor(t, mgr, wh, tgt) require.NoError( t, diff --git a/internal/handlers/target_create.go b/internal/handlers/target_create.go index 5410f47..a8fa834 100644 --- a/internal/handlers/target_create.go +++ b/internal/handlers/target_create.go @@ -9,6 +9,7 @@ import ( "strings" "github.com/go-chi/chi" + "sneak.berlin/go/webhooker/internal/archive" "sneak.berlin/go/webhooker/internal/database" "sneak.berlin/go/webhooker/internal/delivery" ) @@ -27,8 +28,8 @@ func (h *Handlers) HandleTargetCreate() http.HandlerFunc { sourceID := chi.URLParam(r, "sourceID") - h.renameMu.Lock() - defer h.renameMu.Unlock() + h.archives.Lock() + defer h.archives.Unlock() var webhook database.Webhook @@ -361,12 +362,12 @@ func buildDatabaseTargetConfig( ) (string, string, error) { expiry = strings.TrimSpace(expiry) - err := delivery.ValidateArchiveExpiry(expiry) + err := archive.ValidateExpiry(expiry) if err != nil { return "", fmt.Sprintf("Invalid archive expiry: %v", err), nil } - err = delivery.ValidateArchiveRotation(rotation) + err = archive.ValidateRotation(rotation) if err != nil { return "", fmt.Sprintf("Invalid archive rotation: %v", err), nil } diff --git a/internal/handlers/target_download.go b/internal/handlers/target_download.go index 2638d5a..3be8e8a 100644 --- a/internal/handlers/target_download.go +++ b/internal/handlers/target_download.go @@ -6,8 +6,8 @@ import ( "net/http" "time" + "sneak.berlin/go/webhooker/internal/archive" "sneak.berlin/go/webhooker/internal/database" - "sneak.berlin/go/webhooker/internal/delivery" ) // downloadWriteTimeout is how long one write of a download may wait @@ -16,7 +16,7 @@ const downloadWriteTimeout = 60 * time.Second // HandleTargetDownload serves a database target's archive as one // gzipped JSON file, named for the webhook, the target and the time; -// see delivery.ArchiveExport.WriteGzipJSON for what it holds. Other +// see archive.Export.WriteGzipJSON for what it holds. Other // target types have no archive and are a 404. // // A download runs for as long as the client keeps reading: it reads @@ -37,7 +37,7 @@ func (h *Handlers) HandleTargetDownload() http.HandlerFunc { w.Header().Set("Content-Type", "application/gzip") w.Header().Set( "Content-Disposition", - `attachment; filename="`+delivery.ArchiveExportFileName( + `attachment; filename="`+archive.ExportFileName( webhook.Name, target.Name, now, )+`"`, ) @@ -82,17 +82,17 @@ func (d downloadWriter) Write(b []byte) (int, error) { // listTargetArchive lists the archive files of the request's database // target for export. It reports false once it has written the response. // -// It holds renameMu, which every archive rename runs under, while it -// reads the stored names and lists the files, so the files it lists -// are the ones those names give. It lets go before the export is -// streamed, which takes renameMu again for each file only while it +// It holds the archives' lock, which every archive rename runs under, +// while it reads the stored names and lists the files, so the files it +// lists are the ones those names give. It lets go before the export is +// streamed, which takes the lock again for each file only while it // finds the file under the names stored then and opens it. func (h *Handlers) listTargetArchive( w http.ResponseWriter, r *http.Request, -) (database.Webhook, *database.Target, *delivery.ArchiveExport, bool) { - h.renameMu.Lock() - defer h.renameMu.Unlock() +) (database.Webhook, *database.Target, *archive.Export, bool) { + h.archives.Lock() + defer h.archives.Unlock() webhook, target, ok := h.ownedTarget(w, r) if !ok { @@ -105,9 +105,9 @@ func (h *Handlers) listTargetArchive( return database.Webhook{}, nil, nil, false } - export, err := delivery.NewArchiveExport( - delivery.ArchivePath(h.dbMgr, &webhook, target), - &h.renameMu, + export, err := archive.NewExport( + archive.Path(h.dbMgr, &webhook, target), + h.archives, func() (string, error) { return h.storedArchivePath(webhook.ID, target.ID) }, @@ -122,9 +122,9 @@ func (h *Handlers) listTargetArchive( return webhook, target, export, true } -// storedArchivePath returns the path delivery.ArchivePath gives a +// storedArchivePath returns the path archive.Path gives a // database target under the names stored for it and its webhook now. -// Its caller holds renameMu. A webhook or target deleted since is still +// Its caller holds the archives' lock. A webhook or target deleted since is still // found, since deleting one leaves its archive files under their names. func (h *Handlers) storedArchivePath( webhookID, targetID string, @@ -143,5 +143,5 @@ func (h *Handlers) storedArchivePath( return "", err } - return delivery.ArchivePath(h.dbMgr, &webhook, &target), nil + return archive.Path(h.dbMgr, &webhook, &target), nil } diff --git a/internal/handlers/target_download_test.go b/internal/handlers/target_download_test.go index f52b6c7..272fdfc 100644 --- a/internal/handlers/target_download_test.go +++ b/internal/handlers/target_download_test.go @@ -21,9 +21,9 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "sneak.berlin/go/webhooker/internal/archive" "sneak.berlin/go/webhooker/internal/config" "sneak.berlin/go/webhooker/internal/database" - "sneak.berlin/go/webhooker/internal/delivery" "sneak.berlin/go/webhooker/internal/middleware/middlewaretest" ) @@ -223,8 +223,8 @@ func TestHandleTargetDownload_FindsFilesAfterRename(t *testing.T) { env := setupSourceTest(t) wh := seedWebhookWithRetention(t, env.db, 7) - archive := seedTarget(t, env.db, wh.ID, database.TargetTypeDatabase) - oldPath := delivery.ArchivePath(env.dbMgr, &wh, archive) + target := seedTarget(t, env.db, wh.ID, database.TargetTypeDatabase) + oldPath := archive.Path(env.dbMgr, &wh, target) month := func(path string) string { return strings.TrimSuffix(path, ".db") + "-2026-10.db" @@ -233,19 +233,19 @@ func TestHandleTargetDownload_FindsFilesAfterRename(t *testing.T) { seedArchive(t, oldPath, 1, 16) seedArchive(t, month(oldPath), 1, 16) - sw, downloaded := startStalledDownload(t, env, wh.ID, archive.ID) + sw, downloaded := startStalledDownload(t, env, wh.ID, target.ID) require.Equal(t, - http.StatusSeeOther, renameTarget(env, wh.ID, archive.ID).Code, + http.StatusSeeOther, renameTarget(env, wh.ID, target.ID).Code, ) // The test's archives record a rename without moving any file, so - // the files are moved here, as the delivery engine moves them. + // the files are moved here, as the archive writers move them. var renamed database.Target - require.NoError(t, env.db.DB().First(&renamed, "id = ?", archive.ID).Error) + require.NoError(t, env.db.DB().First(&renamed, "id = ?", target.ID).Error) - newPath := delivery.ArchivePath(env.dbMgr, &wh, &renamed) + newPath := archive.Path(env.dbMgr, &wh, &renamed) require.NoError(t, os.Rename(oldPath, newPath)) require.NoError(t, os.Rename(month(oldPath), month(newPath))) @@ -355,16 +355,16 @@ func TestHandleTargetDownload_OutlastsTheRequestLimit(t *testing.T) { env := setupSourceTest(t) wh := seedWebhookWithRetention(t, env.db, 7) - archive := seedTarget(t, env.db, wh.ID, database.TargetTypeDatabase) + target := seedTarget(t, env.db, wh.ID, database.TargetTypeDatabase) seedArchive( - t, delivery.ArchivePath(env.dbMgr, &wh, archive), rows, bodySize, + t, archive.Path(env.dbMgr, &wh, target), rows, bodySize, ) srv, accessLog := limitedServer(t, env, limit) req, err := http.NewRequestWithContext( t.Context(), http.MethodGet, - srv.URL+downloadPath(wh.ID, archive.ID), nil, + srv.URL+downloadPath(wh.ID, target.ID), nil, ) require.NoError(t, err) diff --git a/internal/handlers/target_edit.go b/internal/handlers/target_edit.go index ce57455..94c75af 100644 --- a/internal/handlers/target_edit.go +++ b/internal/handlers/target_edit.go @@ -6,6 +6,7 @@ import ( "strconv" "github.com/go-chi/chi" + "sneak.berlin/go/webhooker/internal/archive" "sneak.berlin/go/webhooker/internal/database" "sneak.berlin/go/webhooker/internal/delivery" ) @@ -96,8 +97,8 @@ func (h *Handlers) HandleTargetEdit() http.HandlerFunc { // HandleTargetEditSubmit handles the target edit form submission. func (h *Handlers) HandleTargetEditSubmit() http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - h.renameMu.Lock() - defer h.renameMu.Unlock() + h.archives.Lock() + defer h.archives.Unlock() webhook, target, ok := h.ownedTarget(w, r) if !ok { @@ -157,7 +158,7 @@ func (h *Handlers) applyTargetEdit( } // A new name renames the archive file before it is saved (see - // delivery.Engine.Rename). If either step fails, it goes back to + // archive.Writers.Rename). If either step fails, it goes back to // the name that is still stored. err = h.renameTargetArchive( target, webhook.Name, target.Name, edited.Name, @@ -178,7 +179,7 @@ func (h *Handlers) applyTargetEdit( ) } - if errors.Is(err, delivery.ErrArchiveNameTaken) { + if errors.Is(err, archive.ErrNameTaken) { h.renderTargetEdit( w, r, webhook, target, in, "Not saved: "+err.Error()+ diff --git a/internal/handlers/target_list.go b/internal/handlers/target_list.go index 6161ee0..9e200b7 100644 --- a/internal/handlers/target_list.go +++ b/internal/handlers/target_list.go @@ -10,6 +10,7 @@ import ( "github.com/dustin/go-humanize" "gorm.io/gorm" + "sneak.berlin/go/webhooker/internal/archive" "sneak.berlin/go/webhooker/internal/database" "sneak.berlin/go/webhooker/internal/delivery" ) @@ -242,7 +243,7 @@ func readTargetDeliveries( func (h *Handlers) archiveFileView( webhook *database.Webhook, target *database.Target, now time.Time, ) *ArchiveFileView { - current, err := delivery.ArchivePathAt(h.dbMgr, webhook, target, now) + current, err := archive.PathAt(h.dbMgr, webhook, target, now) if err != nil { return h.archiveUnreadable(&ArchiveFileView{}, target, err) } @@ -254,8 +255,8 @@ func (h *Handlers) archiveFileView( view.Note = "not created yet" } - files, err := delivery.StatArchive( - delivery.ArchivePath(h.dbMgr, webhook, target), + files, err := archive.Stat( + archive.Path(h.dbMgr, webhook, target), ) switch { diff --git a/internal/handlers/target_list_test.go b/internal/handlers/target_list_test.go index a12fa89..cf244a9 100644 --- a/internal/handlers/target_list_test.go +++ b/internal/handlers/target_list_test.go @@ -10,8 +10,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "sneak.berlin/go/webhooker/internal/archive" "sneak.berlin/go/webhooker/internal/database" - "sneak.berlin/go/webhooker/internal/delivery" "sneak.berlin/go/webhooker/internal/handlers" "sneak.berlin/go/webhooker/internal/logger" "sneak.berlin/go/webhooker/internal/session" @@ -38,10 +38,10 @@ func TestHandleSourceDetail_ShowsArchiveFile(t *testing.T) { t.Cleanup(app.RequireStop) wh := seedWebhook(t, db) - archive := seedTarget(t, db, wh.ID, database.TargetTypeDatabase) + target := seedTarget(t, db, wh.ID, database.TargetTypeDatabase) seedTarget(t, db, wh.ID, database.TargetTypeLog) - path := delivery.ArchivePath(dbMgr, wh, archive) + path := archive.Path(dbMgr, wh, target) body := renderSourceDetailPage(t, h, sess, wh.ID) assert.Equal(t, 1, strings.Count(body, "Archive file:")) diff --git a/internal/handlers/webhook_delete.go b/internal/handlers/webhook_delete.go index 209081d..2660ba9 100644 --- a/internal/handlers/webhook_delete.go +++ b/internal/handlers/webhook_delete.go @@ -77,8 +77,8 @@ func (h *Handlers) deleteWebhookResources( "user_id", userID, ) - // Release the delivery engine's per-webhook archiving state - // so a deleted webhook's archive writer (and any handle open + // Release the webhook's archive writers so a deleted + // webhook's archive writer (and any handle open // within its debounce window) does not linger for the // process lifetime. The archive file itself is deliberately // left on disk; see evictArchiveWriter. @@ -150,9 +150,9 @@ func (h *Handlers) commitWebhookDeletion( return tx.Commit().Error } -// evictArchiveWriter asks the delivery engine to drop the cached -// archive writers of a webhook's database targets, closing their -// archive file handles. +// evictArchiveWriter asks the archive writers to drop the cached +// writers of a webhook's database targets, closing their archive +// file handles. // // The archive database files are NOT deleted. Unlike the event // database — which is per-webhook working storage and is diff --git a/internal/handlers/webhook_edit.go b/internal/handlers/webhook_edit.go index f3ca94b..a7ce564 100644 --- a/internal/handlers/webhook_edit.go +++ b/internal/handlers/webhook_edit.go @@ -6,8 +6,8 @@ import ( "strconv" "github.com/go-chi/chi" + "sneak.berlin/go/webhooker/internal/archive" "sneak.berlin/go/webhooker/internal/database" - "sneak.berlin/go/webhooker/internal/delivery" ) // HandleSourceEdit shows the form to edit a webhook. @@ -59,8 +59,8 @@ func (h *Handlers) HandleSourceEditSubmit() http.HandlerFunc { sourceID := chi.URLParam(r, "sourceID") - h.renameMu.Lock() - defer h.renameMu.Unlock() + h.archives.Lock() + defer h.archives.Unlock() var webhook database.Webhook @@ -130,7 +130,7 @@ func (h *Handlers) applyWebhookEdit( edited.RetentionDays = retentionDays // A new name renames the archive files before it is saved (see - // delivery.Engine.Rename). If either step fails, the same targets' + // archive.Writers.Rename). If either step fails, the same targets' // archives go back to the name that is still stored, without // reading the main database again. targets, err := h.renameWebhookArchives( @@ -150,7 +150,7 @@ func (h *Handlers) applyWebhookEdit( ) } - if errors.Is(err, delivery.ErrArchiveNameTaken) { + if errors.Is(err, archive.ErrNameTaken) { h.renderWebhookEdit( w, r, webhook, name, description, retention, "Not saved: "+err.Error()+ diff --git a/internal/resetpw/resetpw_test.go b/internal/resetpw/resetpw_test.go index fe4fdce..caff47b 100644 --- a/internal/resetpw/resetpw_test.go +++ b/internal/resetpw/resetpw_test.go @@ -10,6 +10,7 @@ import ( "os" "path/filepath" "strings" + "sync" "testing" "time" @@ -17,6 +18,7 @@ import ( "github.com/stretchr/testify/require" "go.uber.org/fx" "go.uber.org/fx/fxtest" + "sneak.berlin/go/webhooker/internal/archive" "sneak.berlin/go/webhooker/internal/config" "sneak.berlin/go/webhooker/internal/database" "sneak.berlin/go/webhooker/internal/datadir" @@ -133,7 +135,9 @@ type noopNotifier struct{} func (n *noopNotifier) Notify([]delivery.Task) {} -type noopArchives struct{} +type noopArchives struct { + sync.Mutex +} func (n *noopArchives) EvictWebhook(string) {} @@ -183,7 +187,7 @@ func newServerApp( healthcheck.New, session.New, func() delivery.Notifier { return &noopNotifier{} }, - func() delivery.Archives { return &noopArchives{} }, + func() archive.Archives { return &noopArchives{} }, func() delivery.CircuitBreakers { return &noopCircuitBreakers{} }, diff --git a/internal/server/routes_test.go b/internal/server/routes_test.go index f7d9b57..ce05c9b 100644 --- a/internal/server/routes_test.go +++ b/internal/server/routes_test.go @@ -11,6 +11,7 @@ import ( "slices" "strconv" "strings" + "sync" "testing" "time" @@ -20,6 +21,7 @@ import ( "go.uber.org/fx" "go.uber.org/fx/fxtest" "gorm.io/gorm/clause" + "sneak.berlin/go/webhooker/internal/archive" "sneak.berlin/go/webhooker/internal/config" "sneak.berlin/go/webhooker/internal/database" "sneak.berlin/go/webhooker/internal/delivery" @@ -50,10 +52,12 @@ type noopNotifier struct{} func (n *noopNotifier) Notify([]delivery.Task) {} -// noopArchives satisfies handlers.New's delivery.Archives +// noopArchives satisfies handlers.New's archive.Archives // dependency. No test here checks what gets evicted or renamed, so -// it records nothing. -type noopArchives struct{} +// it records nothing. Its lock is a plain mutex. +type noopArchives struct { + sync.Mutex +} func (e *noopArchives) EvictWebhook(string) {} @@ -138,7 +142,7 @@ func newTestEnvWithConfig( healthcheck.New, session.New, func() delivery.Notifier { return &noopNotifier{} }, - func() delivery.Archives { return &noopArchives{} }, + func() archive.Archives { return &noopArchives{} }, func() delivery.CircuitBreakers { return &noopCircuitBreakers{} },