Skip to content

Transactional: a failed Commit still returns the handler's 2xx, and one panicking commit hook silently skips the rest #364

Description

@FrameAutomata

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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions