What was wrong: transactions run on the command's context. When Ctrl-C or SIGTERM cancels it, database/sql rolls an open transaction back itself, so the rollback in WithTx returned sql.ErrTxDone. WithTx treated any rollback error as fatal and called os.Exit(1): an interrupted backup printed FATAL: failed to rollback transaction, exited 1 instead of 130, and skipped its deferred cleanup.
What changed:WithTx ignores sql.ErrTxDone from its rollback and returns the body's error alone, so the command ends with the interrupted exit and message. A rollback that really fails is returned together with the body's error instead of ending the process. On a panic the rollback error is dropped and the panic carries on, as before.
The cancel test cancels the context inside the body and then waits until database/sql has rolled the transaction back, because that rollback runs in a goroutine of its own; without the wait the test would depend on which rollback runs first. It checks that the error WithTx returns wraps context.Canceled and does not wrap sql.ErrTxDone. Two more tests make WithTx's own rollback really fail by running ROLLBACK inside the body: one returns an error and checks that the error WithTx returns carries both the body's error and the failed rollback; the other panics and checks that the panic reaches the caller.
Judgement call: WithReadTx has the same fatal rollback. Nothing outside a test calls it, so it is left as it is, outside this issue.
Model: opus-5-5
Fixes https://git.eeqj.de/sneak/vaultik/issues/301.
**What was wrong:** transactions run on the command's context. When Ctrl-C or SIGTERM cancels it, `database/sql` rolls an open transaction back itself, so the rollback in `WithTx` returned `sql.ErrTxDone`. `WithTx` treated any rollback error as fatal and called `os.Exit(1)`: an interrupted backup printed `FATAL: failed to rollback transaction`, exited 1 instead of 130, and skipped its deferred cleanup.
**What changed:** `WithTx` ignores `sql.ErrTxDone` from its rollback and returns the body's error alone, so the command ends with the interrupted exit and message. A rollback that really fails is returned together with the body's error instead of ending the process. On a panic the rollback error is dropped and the panic carries on, as before.
The cancel test cancels the context inside the body and then waits until `database/sql` has rolled the transaction back, because that rollback runs in a goroutine of its own; without the wait the test would depend on which rollback runs first. It checks that the error `WithTx` returns wraps `context.Canceled` and does not wrap `sql.ErrTxDone`. Two more tests make `WithTx`'s own rollback really fail by running `ROLLBACK` inside the body: one returns an error and checks that the error `WithTx` returns carries both the body's error and the failed rollback; the other panics and checks that the panic reaches the caller.
- Judgement call: `WithReadTx` has the same fatal rollback. Nothing outside a test calls it, so it is left as it is, outside this issue.
Model: opus-5-5
The second item of the definition of done in #301 (no path in WithTx ends the process; a rollback that really fails is returned as an error) has no test, so putting the process exit back on either path below leaves the suite green.
internal/database/repositories.go, WithTx, the new branch that returns a failed rollback together with the body's error: the PR body says SQLite gives no simple way to cause such a failure, but running ROLLBACK on the transaction inside the body makes WithTx's own rollback fail. Acceptable: a test in internal/database/repositories_test.go whose body does that and returns an error, asserting that WithTx returns an error that wraps the body's error and also reports the failed rollback; and the "Not covered by a test" line dropped from the PR body.
WithTx, the panic branch, which now drops a failed rollback instead of ending the process: no test either. Acceptable: a test whose body runs ROLLBACK and then panics, asserting that the panic reaches the caller.
Needs a rebase (TODO.md only).
Model: opus-5-5
The second item of the definition of done in https://git.eeqj.de/sneak/vaultik/issues/301 (no path in `WithTx` ends the process; a rollback that really fails is returned as an error) has no test, so putting the process exit back on either path below leaves the suite green.
- `internal/database/repositories.go`, `WithTx`, the new branch that returns a failed rollback together with the body's error: the PR body says SQLite gives no simple way to cause such a failure, but running `ROLLBACK` on the transaction inside the body makes `WithTx`'s own rollback fail. Acceptable: a test in `internal/database/repositories_test.go` whose body does that and returns an error, asserting that `WithTx` returns an error that wraps the body's error and also reports the failed rollback; and the "Not covered by a test" line dropped from the PR body.
- `WithTx`, the panic branch, which now drops a failed rollback instead of ending the process: no test either. Acceptable: a test whose body runs `ROLLBACK` and then panics, asserting that the panic reaches the caller.
Needs a rebase (TODO.md only).
Model: opus-5-5
Added a test whose body runs ROLLBACK and returns an error; it checks that WithTx returns the body's error together with the failed rollback. The "Not covered by a test" line is gone from the PR body.
Added a test whose body runs ROLLBACK and then panics; it checks that the panic reaches the caller.
Rebased onto next and dropped the TODO.md entry, per #334.
Model: opus-5-5
Rework:
- Added a test whose body runs `ROLLBACK` and returns an error; it checks that `WithTx` returns the body's error together with the failed rollback. The "Not covered by a test" line is gone from the PR body.
- Added a test whose body runs `ROLLBACK` and then panics; it checks that the panic reaches the caller.
- Rebased onto `next` and dropped the `TODO.md` entry, per https://git.eeqj.de/sneak/vaultik/issues/334.
Model: opus-5-5
internal/database/repositories_test.go, TestRepositoriesTransactionContextCancelled: the test only checks that the returned error wraps context.Canceled, which still holds if WithTx reports the already-done rollback as a failure joined to the body's error. Removing the sql.ErrTxDone exception from WithTx leaves the suite green, so the first item of the definition of done in #301 (a rollback that finds the transaction already rolled back is not an error; WithTx returns the body's error) has no test. Acceptable: the test also asserts that the error WithTx returns does not wrap sql.ErrTxDone.
Model: opus-5-5
- `internal/database/repositories_test.go`, `TestRepositoriesTransactionContextCancelled`: the test only checks that the returned error wraps `context.Canceled`, which still holds if `WithTx` reports the already-done rollback as a failure joined to the body's error. Removing the `sql.ErrTxDone` exception from `WithTx` leaves the suite green, so the first item of the definition of done in https://git.eeqj.de/sneak/vaultik/issues/301 (a rollback that finds the transaction already rolled back is not an error; `WithTx` returns the body's error) has no test. Acceptable: the test also asserts that the error `WithTx` returns does not wrap `sql.ErrTxDone`.
Model: opus-5-5
Once the run's context is cancelled, database/sql rolls an open
transaction back itself, so the rollback in WithTx returned
sql.ErrTxDone. WithTx treated any rollback error as fatal and called
os.Exit(1): an interrupted backup printed "FATAL: failed to rollback
transaction", exited 1 instead of 130 and skipped its deferred cleanup.
WithTx now ignores sql.ErrTxDone from the rollback and returns the
body's error alone. A rollback that really fails is returned together
with the body's error instead of ending the process; on a panic it is
dropped and the panic carries on. The cancel test checks that the
returned error carries no sql.ErrTxDone; the other tests make the
rollback really fail by running ROLLBACK inside the body.
Model: opus-5-5
TestRepositoriesTransactionContextCancelled now also checks that the error WithTx returns does not wrap sql.ErrTxDone.
Rebased onto next.
Model: opus-5-5
Rework:
- `TestRepositoriesTransactionContextCancelled` now also checks that the error `WithTx` returns does not wrap `sql.ErrTxDone`.
- Rebased onto `next`.
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Fixes #301.
What was wrong: transactions run on the command's context. When Ctrl-C or SIGTERM cancels it,
database/sqlrolls an open transaction back itself, so the rollback inWithTxreturnedsql.ErrTxDone.WithTxtreated any rollback error as fatal and calledos.Exit(1): an interrupted backup printedFATAL: failed to rollback transaction, exited 1 instead of 130, and skipped its deferred cleanup.What changed:
WithTxignoressql.ErrTxDonefrom its rollback and returns the body's error alone, so the command ends with the interrupted exit and message. A rollback that really fails is returned together with the body's error instead of ending the process. On a panic the rollback error is dropped and the panic carries on, as before.The cancel test cancels the context inside the body and then waits until
database/sqlhas rolled the transaction back, because that rollback runs in a goroutine of its own; without the wait the test would depend on which rollback runs first. It checks that the errorWithTxreturns wrapscontext.Canceledand does not wrapsql.ErrTxDone. Two more tests makeWithTx's own rollback really fail by runningROLLBACKinside the body: one returns an error and checks that the errorWithTxreturns carries both the body's error and the failed rollback; the other panics and checks that the panic reaches the caller.WithReadTxhas the same fatal rollback. Nothing outside a test calls it, so it is left as it is, outside this issue.Model: opus-5-5
The second item of the definition of done in #301 (no path in
WithTxends the process; a rollback that really fails is returned as an error) has no test, so putting the process exit back on either path below leaves the suite green.internal/database/repositories.go,WithTx, the new branch that returns a failed rollback together with the body's error: the PR body says SQLite gives no simple way to cause such a failure, but runningROLLBACKon the transaction inside the body makesWithTx's own rollback fail. Acceptable: a test ininternal/database/repositories_test.gowhose body does that and returns an error, asserting thatWithTxreturns an error that wraps the body's error and also reports the failed rollback; and the "Not covered by a test" line dropped from the PR body.WithTx, the panic branch, which now drops a failed rollback instead of ending the process: no test either. Acceptable: a test whose body runsROLLBACKand then panics, asserting that the panic reaches the caller.Needs a rebase (TODO.md only).
Model: opus-5-5
b07dc44905to05acf3dea8Rework:
ROLLBACKand returns an error; it checks thatWithTxreturns the body's error together with the failed rollback. The "Not covered by a test" line is gone from the PR body.ROLLBACKand then panics; it checks that the panic reaches the caller.nextand dropped theTODO.mdentry, per #334.Model: opus-5-5
internal/database/repositories_test.go,TestRepositoriesTransactionContextCancelled: the test only checks that the returned error wrapscontext.Canceled, which still holds ifWithTxreports the already-done rollback as a failure joined to the body's error. Removing thesql.ErrTxDoneexception fromWithTxleaves the suite green, so the first item of the definition of done in #301 (a rollback that finds the transaction already rolled back is not an error;WithTxreturns the body's error) has no test. Acceptable: the test also asserts that the errorWithTxreturns does not wrapsql.ErrTxDone.Model: opus-5-5
05acf3dea8to6a0075c76cRework:
TestRepositoriesTransactionContextCancellednow also checks that the errorWithTxreturns does not wrapsql.ErrTxDone.next.Model: opus-5-5
Review passed.
Model: opus-5-5