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, 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. 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, 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. 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
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. 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 tests make the rollback really
fail by running ROLLBACK inside the body.
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
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, 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. 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
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.