1.0/mvp #15
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: sneak/webhooker#15
Reference in New Issue
Block a user
@clawbot
hi sdlc-manager. i'm about to go to sleep. i'd like you to drive webhooker to mvp/1.0 feature completion while i'm asleep. i'd like there to be ONE pr for it. you can make sub-issues (and in fact i believe there are some already) to track TODOs but this issue is going to be the one the PR is for, and you can make commits on that PR branch (mark the PR WIP until you think it's actually polished and ready and debugged and feature complete) that close those sub-issues, but the main PR will close this one.
you're to do all of it. don't stop until it's done. do periodic bug finding audit passes. do refactoring passes. do security review passes. go all of the way. i have faith in you, your subagents, and the README.
i recommend doing code review passes after each subtask, and doing any required rework before proceeding to the next subtask.
[manager] On it. Here's my plan:
Phase 1 — Foundation (dispatching now):
Phase 2 — Core Webhook Engine:
Phase 3 — Web UI:
Phase 4 — Security & Polish:
Each phase builds on the previous branch. Will update this issue as phases complete.
cool. when doing the UI look at µPaaS for an example, and follow its styles and layout conventions, because I like the way it looks.
clawbot referenced this issue2026-03-02 00:56:20 +01:00
Phase 1: Foundation — Complete
PR: #16
What was done:
Entity rename (closes #12) — Renamed
Processor→WebhookandWebhook→Entrypointthroughout the entire codebase: database models, handlers, routes, comments, README.go:embed for templates (closes #7) — Templates are now embedded using
//go:embedand parsed once at startup withtemplate.Must(template.ParseFS(...))instead of re-parsing from disk on every request.slog.LevelVar (closes #8) — Logger now uses
slog.LevelVarfor dynamic log level changes without recreating the handler.Env var config (closes #10) — Configuration now prefers environment variables over config.yaml values. Each config field has a corresponding env var that takes precedence.
Remove redundant godotenv (closes #11) —
godotenv/autoloadimport is now only inconfig.go.Auth middleware (closes #9) — Added
RequireAuthsession-based middleware to protect/sourcesand/source/{sourceID}routes. Unauthenticated users are redirected to/pages/login.MIT LICENSE — Added MIT license file.
All commits pass
docker build .(which runsmake check: format, lint, test, build).Ready for Phase 2: core webhook engine.
Phase 2 Complete: Core Webhook Engine + Delivery + Management UI
All work committed to
feature/mvp-1.0branch, PR #16 updated.What was built:
Webhook Reception Handler (
/webhook/{uuid}):Delivery Engine (
internal/delivery/engine.go):Target Type Implementations:
Webhook Management Pages (Tailwind CSS + Alpine.js):
/sources): all webhooks with entrypoint/target/event counts/sources/new): form with name, description, retention_days; auto-creates default entrypoint/source/{id}): config, entrypoints with URLs, targets with config, recent events/source/{id}/edit): update name, description, retention_days/source/{id}/delete): soft-delete webhook + all child records/source/{id}/entrypoints): inline form with Alpine.js toggle/source/{id}/targets): type-aware form (shows URL field for http/retry, max_retries for retry)/source/{id}/logs): paginated (25/page), expandable event bodies, delivery status per targetREADME Updates:
Quality:
docker build .passes all checks (fmt-check ✅, lint ✅, test ✅, build ✅)QA Test Report —
feature/mvp-1.0BranchDate: 2026-03-01
Branch:
feature/mvp-1.0Tester: clawbot (automated QA)
Build Status
make build(local)docker build .(CI gate)go test -v -race ./...Test Coverage
internal/configinternal/databaseinternal/globalsinternal/loggerinternal/handlersinternal/deliveryinternal/middlewareinternal/serverinternal/sessioninternal/healthcheckpkg/configRuntime Test Results
Working ✅
/.well-known/healthcheck) returns valid JSON with status, uptime, version{"status":"ok"}Critical Failures ❌
Application crashes ~2 seconds after startup — nil pointer dereference in delivery engine. The
Engine.New()constructor callsparams.DB.DB()before the database connection is established. See #17.All HTML pages render as blank — Every template-rendered page (index, login, sources, detail, edit, logs) returns HTTP 200 with empty body. The
template.Execute()runs the wrong root template. See #18.DevSessionKey is wrong size — The dev session key constant decodes to 35 bytes but Session requires 32. Running without a manually-created SESSION_KEY fails. See #19.
Security Audit
crypto/randwithmath/bigto avoid modulo bias*origin — CHANGEME comment left in code. See #23Issues Created
Blockers (3)
Should-Fix (5)
Should-Fix — UX (2)
Nice-to-Have (2)
Overall Assessment
The
feature/mvp-1.0branch is NOT ready for release. There are 3 blocking issues that prevent the application from functioning:What's working well:
make check) all pass cleanlyWhat needs attention after blockers:
Bottom line: The architecture and individual components are sound, but the integration between them has critical bugs. The two main blockers (#17 and #18) are straightforward fixes — one is a lifecycle ordering bug (store wrapper not value), the other is template parse order (swap file order in ParseFS). Once those two are fixed, the core webhook flow should work end-to-end.
Phase 3 Complete: Bug Fixes + Security Hardening
All 3 blockers, 7 should-fix issues, and the owner feedback item have been resolved on branch
feature/mvp-1.0(PR #16).Blockers Fixed
*database.Databasewrapper, call.DB()lazilyParseFSargs so page file is firstShould-Fix Resolved
cleanShutdown()callDevAdminUsername/DevAdminPasswordconfigOwner Feedback
archived_eventstable (newArchivedEventmodel) that persists independently of internal event retentionVerification
docker build .passes (fmt-check ✓, lint ✓, tests ✓, build ✓).golangci.yml,Makefile, or CI configQA Test Report: Per-Webhook Event Database Implementation
Branch:
feature/mvp-1.0| PR: PR #16 | Date: 2026-03-021. Build Verification ✅ PASS
docker build .(fmt-check, lint, test, build)make build(local binary)2. Architecture Review ✅ PASS
WebhookDBManager (
internal/database/webhook_db_manager.go):sync.Map(thread-safe cache)GetDB()with race-safeLoadOrStore(duplicate connections handled correctly)CloseAll()registered onOnStopos.MkdirAllwith0750permissions on initializationMain DB migration (
internal/database/models.go):User,APIKey,Webhook,Entrypoint,TargetArchivedEvent removal:
ArchivedEventorarchived_eventanywhere in the codebaseCross-DB safety:
Preloadcall in production code (Preload("Event")in delivery engine) — correct, same-DBDelivery engine (
internal/delivery/engine.go):DBExists()before openingDI wiring (
cmd/webhooker/main.go):database.NewWebhookDBManagerproperly provided alongsidedatabase.NewWebhookDBMgrvia fx injection3. Functional Test Results ✅ PASS
events-{uuid}.dbcreated in DATA_DIR{"status":"ok"}delivered4. Edge Case Results
os.MkdirAllcreates nested paths with 0750 permissionsCloseAll()fires correctly in fx lifecycle, all per-webhook DBs closed*gorm.DBdoes not detect file deletionNote on the stale-cache edge case: When a per-webhook DB file is deleted from disk while its connection remains cached in the
sync.Map,GetDB()returns the stale cached connection, which fails with "attempt to write a readonly database (1032)". This is not a normal user flow (it requires external file manipulation), but a health check or retry-with-reopen mechanism could improve robustness. Low priority.5. README Accuracy ✅ PASS
webhook_db_manager.go6. Issues Found
No new blocking issues found. One minor enhancement opportunity (stale DB connection cache — low priority, not a normal user flow).
7. Overall Verdict: ✅ READY for formal code review
The per-webhook event database implementation is well-structured, fully functional, and production-ready. Key strengths:
sync.MapwithLoadOrStoreprevents duplicate connectionsRecommend proceeding with formal code review via PR #16.
why does the delivery engine do polling when this is go and we have channels and goroutines?
Good point — the delivery engine currently uses a 2-second polling ticker to query for pending deliveries. This should be event-driven using Go channels:
HandleWebhookcreates delivery records, it signals the engine via a channelDispatching rework to convert the delivery engine from polling to channel-based.
the backoff/retry timer can be a go timer that shoots the event id and retry count into a channel, the receiver for which updates the db when retrying delivery. this way the db is only used for inter-component communication on unexpected restart.
make sure all channel usage is safe and idiomatic.
Agreed — much cleaner design. Updated the rework agent with your guidance:
time.Timerfor the backoff duration that fires the delivery info into a retry channelwhy are we passing in the season key? it should be stored in the db and a secure value randomly generated and stored on first run if one doesn’t already exist. no need to put that burden on the user.
Agreed — the session key should be auto-managed:
settingsorconfigtable)SESSION_KEYfrom required env vars and configDevSessionKeyfor dev mode only (or remove it entirely if the auto-generated key works for dev too)Will implement this after the delivery engine rework completes.
the delivery info should not include the body unless the body is <16KB. make the body struct member be a pointer and if it’s null use the event id to fetch the body from the db. this way we don’t use up tons of memory in a high traffic situation.
describe for me and document the exact flow an event takes, through entrypoint to delivery. give me examples of the case where everything works first try, where the delivery needs to be retried once, and where the target is offline for 12 hours. include timing on all steps.
Event Flow: Entrypoint → Delivery
Describing the channel-based architecture currently being implemented.
Scenario 1: Happy Path (HTTP target, first try succeeds)
/webhook/{uuid}WebhookDBManager.GetDB(webhookID)Eventrecord (method, headers, body, content-type)Targetsfrom main DBDeliveryrecords (one per target, status=pending)engine.Notify(webhookID)— sends to delivery channel (non-blocking)200 OKwith{"status":"ok"}to callerDeliveryResult(success, attempt 1, 30ms duration)Deliverystatus →deliveredTotal latency: ~45ms from webhook received to delivery confirmed.
Scenario 2: Retry Target, Succeeds on Second Attempt
DeliveryResult(fail, attempt 1, status 503)Deliverystatus →retryingtime.Timerfor 1s backoff (2^0) — fires retry info into retry channel{deliveryID, eventID, webhookID, attempt: 2}to retry channelDeliveryResult(success, attempt 2, 30ms)Deliverystatus →deliveredTotal: ~6 seconds (1s backoff + network time).
Scenario 3: Retry Target, Target Offline for 12 Hours
With default
max_retries=5and exponential backoff:Deliverystatus →failed⚠️ With default max_retries=5, all retries exhaust in ~15 seconds. The 12-hour offline scenario can't succeed with 5 retries.
For a 12-hour recovery window, the user would need to set higher
max_retries. With a backoff cap (which we should add — currently uncapped):Recommendation: Add a configurable
max_backoff(default: 5 minutes) per retry target. With capped backoff, the retry window becomes roughlymax_retries × max_backoff.Recovery on Unexpected Restart
On startup, the delivery engine scans all per-webhook databases for deliveries in
pendingorretryingstatus. These are re-queued into the delivery/retry channels, so interrupted deliveries resume automatically. The DB is the durable recovery mechanism; channels are the fast path.Memory Optimization
The delivery channel payload includes the event body as a
*string. If the body is < 16KB, it's included inline. If ≥ 16KB, the pointer is nil and the engine fetches it from the per-webhook DB on demand. This prevents large payloads from consuming memory while sitting in the channel buffer.in the happy path the delivery agent needs to trust the endpoint handler did the writes properly and isn’t lying, the message in the <=16KB case should have everything it needs to do its delivery. it shouldn’t touch the db until it has a success or failure to record.
Makes sense — in the ≤16KB case, the notification should be fully self-contained:
The engine trusts the handler and goes straight to delivery without touching any DB. It only writes to the DB to record the DeliveryResult and update Delivery status after the attempt.
For the >16KB case, it still needs to fetch the body from the per-webhook DB, but that's the exceptional path.
Will implement in the next pass after the current session key rework completes.
each event needs to fan out to each of the webhook’s tatgets in parallel as quickly as possible in the handler. the per-event target notifications/processint should be in separate goroutines. the durable target needs a circuit breaker.
Implementing both:
Parallel fan-out: Each event's targets get delivered in separate goroutines simultaneously. Log, HTTP, retry, and database targets all start at the same time.
Circuit breaker for retry targets:
Rework agent dispatched.
How do we prevent a huge flood in parallel in the case where the circuit breaker is open a long time and there are many queued messages?
Good catch — thundering herd problem. If the circuit is open for hours and 1000 events queue up, they'd all try to deliver the moment the circuit closes.
Current behavior: when the circuit is Open, failed deliveries get retry timers. Those timers fire into the retry channel, which tries to deliver. But with the circuit still open, they get rescheduled. When the half-open probe succeeds and the circuit closes, all pending retry timers start flowing through at once.
Proposed fix: per-target concurrency limiter.
Add a semaphore per target (e.g., max 10 concurrent in-flight deliveries to the same target). When a delivery goroutine starts, it acquires the semaphore. When done, it releases. If the semaphore is full, the goroutine blocks until a slot opens.
This naturally rate-limits the flood:
The semaphore could be configurable per target (defaulting to 10), or just a sensible global default. Implementation:
make(chan struct{}, maxConcurrency)per target in async.Map.Should I implement this now, or is the current PR ready for merge and this can be a follow-up?
i think the retry handler should be renamed http and the http case (fire and forget) should simply be retries=0 in the target event
implement the semaphore, though do we really want to use 20000 waiting goroutines as our queue?
Right — 20k goroutines blocking on a semaphore is wasteful. Better approach:
The retry channel itself is the queue. Instead of spawning a goroutine per retry that blocks on a semaphore, keep the retries in the channel and have a fixed worker pool per the engine:
This means at most N goroutines are actively delivering at any time, and the channels serve as bounded queues. No goroutine explosion.
Dispatching now.
if the retry channel is full, we shouldn’t be bothering with timers, we should fall back to a db-mediated path. with a big backlog we run into the same 20k goroutines problem.
Agreed — bounded channels alone just move the goroutine pileup from the pool to the timers. The fix:
retry_aftertimestamp. No timer, no goroutine.This gives us bounded memory (channel buffer + DB), zero goroutine growth under backpressure, and crash recovery for free (DB retries survive restart). Dispatching rework now.
This is already implemented in the current code (commit
10db6c5). Here's how it works:scheduleRetryusestime.AfterFunc+ non-blocking channel send:When the retry channel is full, the timer goroutine returns immediately (no blocking, no re-arm). The delivery record stays in
retryingstatus in the per-webhook DB.retrySweep(DB-mediated fallback) runs every 60s:status = retryingSo under backpressure: zero blocked goroutines, zero unbounded timers. The DB is the durable overflow queue, sweep is the drain.
The latest commit (
4dd4dfa) also cleaned up DBURL→DATA_DIR consolidation, dead code, stale comments, and README accuracy for 1.0 readiness. Dispatching a reviewer now.