Two problems in middleware.Transactional (backend/app/middleware/transactional.middleware.go), found while reviewing #363. Both are pre-existing and neither is introduced by that PR, so filing separately.
1. A failed Commit() still delivers the 2xx the handler already wrote
if status := c.Writer.Status(); status >= 200 && status < 400 {
if err := txHandle.Commit(); err != nil {
c.AbortWithStatus(http.StatusInternalServerError) // no-op: headers already written
panic(err) // swallowed by gin.Recovery
}
runCommitHooks(c)
} else {
txHandle.Rollback()
}
By the time Transactional decides to commit, the handler has already called c.JSON(...), so gin has written the status and body. c.AbortWithStatus(500) then hits gin's guard in responseWriter.WriteHeader — it returns early once Written() — and the panic(err) is caught by gin.Recovery() (cmd/run.go:198, and gin.Default() at :200), which only calls AbortWithStatus and does not re-panic. The buffered 2xx is flushed to the client.
Demonstrated against the real middleware, forcing Commit() to fail the way a locked or disconnected database would:
[Recovery] panic recovered:
Transactional.func1: panic(r)
Transactional: panic(err)
client received: status=201 body={"token":"eyJhbGciOi...signed-jwt"}
rows actually persisted: 0
Impact. POST /api/register is the worst case: the browser stores a valid signed JWT and a project token for a user, organization and project that were never persisted. The UI believes the account exists and every subsequent call fails. /api/login and the invitation-accept routes have the same shape.
Realistic triggers are ordinary operational failures, not exotic ones — SQLite database is locked or a full disk, a PostgreSQL connection dropped between the last statement and COMMIT.
Fix direction. A status tweak cannot work, because the response is already on the wire. The transaction has to commit before the handler renders, or rendering has to be deferred until after the commit — e.g. buffer the response body in the middleware and only flush it once Commit() succeeds.
The same swallow applies to the recover() path at line 24.
2. runCommitHooks has no per-hook recover
func runCommitHooks(c *gin.Context) {
hooks, _ := c.Get(commitHooksContextKey)
fns, _ := hooks.([]func())
for _, fn := range fns {
fn()
}
}
If one hook panics, the remaining hooks for that request never run, and the panic unwinds into Transactional's deferred recover(), which calls Rollback() on an already-committed transaction (silently sql.ErrTxDone, the return value is discarded), then aborts with a status that can no longer be written, then re-panics into gin.Recovery.
The net result is a request whose data is committed but which is logged as a panic, with an arbitrary suffix of its side effects dropped.
This mattered less when hooks were just channel sends (outbox.Wake, synthetics.Wake). Three of the five call sites are now cache.ProjectCache writes, so a handler can legitimately queue "publish to the cache" and "wake a worker" and lose the second.
Fix direction. Wrap each fn in its own recover() plus traceway.CaptureException, so one bad hook cannot cancel the others or corrupt the request's outcome.
Related, not filed here
OnCommit silently discards its callback when the route does not carry Transactional — no error, no log. That is a footgun rather than a live bug (every current caller is on a transactional route), but it is cheap to detect: db.TransactionContextKey is absent from the context in exactly that case.
OnCommit/runCommitHooks had no test coverage at all until #363 added backend/app/middleware/commit_hooks_test.go; that file is a reasonable place to hang regression tests for both problems above.
Two problems in
middleware.Transactional(backend/app/middleware/transactional.middleware.go), found while reviewing #363. Both are pre-existing and neither is introduced by that PR, so filing separately.1. A failed
Commit()still delivers the 2xx the handler already wroteBy the time
Transactionaldecides to commit, the handler has already calledc.JSON(...), so gin has written the status and body.c.AbortWithStatus(500)then hits gin's guard inresponseWriter.WriteHeader— it returns early onceWritten()— and thepanic(err)is caught bygin.Recovery()(cmd/run.go:198, andgin.Default()at:200), which only callsAbortWithStatusand does not re-panic. The buffered 2xx is flushed to the client.Demonstrated against the real middleware, forcing
Commit()to fail the way a locked or disconnected database would:Impact.
POST /api/registeris the worst case: the browser stores a valid signed JWT and a project token for a user, organization and project that were never persisted. The UI believes the account exists and every subsequent call fails./api/loginand the invitation-accept routes have the same shape.Realistic triggers are ordinary operational failures, not exotic ones — SQLite
database is lockedor a full disk, a PostgreSQL connection dropped between the last statement andCOMMIT.Fix direction. A status tweak cannot work, because the response is already on the wire. The transaction has to commit before the handler renders, or rendering has to be deferred until after the commit — e.g. buffer the response body in the middleware and only flush it once
Commit()succeeds.The same swallow applies to the
recover()path at line 24.2.
runCommitHookshas no per-hook recoverIf one hook panics, the remaining hooks for that request never run, and the panic unwinds into
Transactional's deferredrecover(), which callsRollback()on an already-committed transaction (silentlysql.ErrTxDone, the return value is discarded), then aborts with a status that can no longer be written, then re-panics intogin.Recovery.The net result is a request whose data is committed but which is logged as a panic, with an arbitrary suffix of its side effects dropped.
This mattered less when hooks were just channel sends (
outbox.Wake,synthetics.Wake). Three of the five call sites are nowcache.ProjectCachewrites, so a handler can legitimately queue "publish to the cache" and "wake a worker" and lose the second.Fix direction. Wrap each
fnin its ownrecover()plustraceway.CaptureException, so one bad hook cannot cancel the others or corrupt the request's outcome.Related, not filed here
OnCommitsilently discards its callback when the route does not carryTransactional— no error, no log. That is a footgun rather than a live bug (every current caller is on a transactional route), but it is cheap to detect:db.TransactionContextKeyis absent from the context in exactly that case.OnCommit/runCommitHookshad no test coverage at all until #363 addedbackend/app/middleware/commit_hooks_test.go; that file is a reasonable place to hang regression tests for both problems above.