middleware: buffer the response until Commit succeeds; isolate commit hooks - #365
Open
FrameAutomata wants to merge 1 commit into
Open
middleware: buffer the response until Commit succeeds; isolate commit hooks#365FrameAutomata wants to merge 1 commit into
FrameAutomata wants to merge 1 commit into
Conversation
… hooks Transactional decided whether to commit only after the handler had already flushed its response, so a failed Commit() (SQLite "database is locked", a full disk, a dropped Postgres connection) still delivered the handler's 2xx for rows that were never persisted; /api/register handed the browser a JWT for a user that did not exist. The recover path had the same flaw for a handler that panicked after writing. The middleware now swaps c.Writer for a responseBuffer that records the status and body in memory and releases them only after Commit() succeeds. A failed commit discards the buffer, restores the headers as they were before the handler ran, and answers 500 with an empty body through c.AbortWithError so the failure is logged and reported. Commit hooks now run one at a time under their own recover: the transaction is already committed, so a panicking hook is recorded on c.Errors and the remaining hooks and the response still go out. Closes #364 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #364
What
middleware.Transactionaldecided whether to commit only after the handler had already flushed its response, so a failedCommit()(SQLitedatabase is locked, a full disk, a dropped PostgreSQL connection beforeCOMMIT) still delivered the handler's 2xx for rows that were never persisted.POST /api/registerwas the worst case: the browser stored a JWT and a project token for a user, org and project that did not exist. Therecover()path had the same flaw for a handler that panicked after writing.Separately,
runCommitHooksranOnCommitcallbacks with no per-hook recover, so one panicking hook skipped the rest and turned a request whose data was committed into a logged panic.How
backend/app/middleware/buffer_response.go(new):responseBufferreplacesc.Writerfor the handler chain and records the status and body in memory, mirroring gin's ownresponseWriterbookkeeping (Status/Size/Written, status locked once written).Unwrap()keepshttp.ResponseControllerreaching the connection.Hijackreturns an error.Transactional: installs the buffer afterBegin(). On a successful commit it runs the hooks and then releases the buffer with one status and one write. On a failed commit it discards the buffer, restores the headers to what they were before the handler ran (so aContent-Type,LocationorSet-Cookiemeant for the discarded response does not ride out on the 500), and answers throughc.AbortWithError(500, traceway.NewStackTraceErrorf(...)), which lands now because nothing has reached the wire yet. A handler panic after writing discards the same way before re-panicking. ABegin()failure now also goes throughAbortWithErrorinstead ofpanic.OnCommitcallback runs under its own recover; a panic is recorded viac.Error(reported by tracewaygin with the request's trace and printed by gin's Logger even when monitoring is off, whereastraceway.CaptureExceptionis a no-op until the SDK is initialised) and the remaining hooks and the response still go out. Hooks run before the release so the project cache holds the row before the client can send its next request.transactional_test.go): adriver.Connectorwrapping the real modernc sqlite driver whose transactions fail to commit on demand (rolling back for real, so the single in-memory connection is not discarded and a "0 rows" assertion cannot pass vacuously). Cases: failed commit → 500, empty body, handler headers stripped, outer middleware header kept, row absent, hook not run, onec.Errorsentry; successful commit for JSON, redirect, barec.Status(204)andc.JSON(204, nil)with the hook observed running before the first header write; panic after write → 500 and rollback; panicking hook → second hook runs, 201 delivered;Unwrapkeepshttp.ResponseControllerworking. Each case was checked to fail when its guard is removed.c.Erroris the right non-stopping channel.Verification
From
nix develop .#backend, inbackend/:gofmt -l,go vet,go test -race -count=1 ./..., andgo buildunder the default,transactional_pg telemetry_chandtelemetry_duckdbtag sets all pass. The new test file is tagged!transactional_pgonly, so CI'stelemetry_duckdbrun covers it too.Follow-ups (not in this PR)
OnCommiton a route withoutTransactionalsilently discards its callback (listed in Transactional: a failed Commit still returns the handler's 2xx, and one panicking commit hook silently skips the rest #364 under "related, not filed here").Flush()underTransactionalis a silent no-op by design. Nothing on the current transactional routes calls it, but making it loud (an error onc.Errors, or a panic the middleware turns into rollback + 500) would make the "no streaming" rule self-enforcing.release()knows the exact body length and could setContent-Lengthso bodies over 2KB stop going out chunked. Not a regression (gin never set it either), so left out of a bug-fix PR.Conflicts
#363 rewrites the
OnCommitdoc comment in the same file; this PR leaves that comment byte-identical and editsrunCommitHooksdirectly below it, so whichever lands second may see a trivial adjacent-hunk conflict.🤖 Generated with Claude Code