Pins the close that the idle archive sweep runs before it reopens the archive file. A new test keeps the archive's connection from before a sweep and checks that the sweep closed it. Without that close, the reopen replaced the handle without closing it, and TestArchiveSweep_LeavesArchiveClosed could not tell, because the sweep then closed the new handle; one connection would leak per archive per sweep.
The sweeper's query listing database targets now takes the sweep's context. A sweep cancelled by the app stopping, before or during that query, returns without an error line, because stopping is not a failure; a test runs a sweep on a cancelled context and checks that nothing is logged at error level. A comment on the sweeper's cancel function says why it needs no lock: fx calls the stop hook only after the start hook has returned. The retention reaper relies on the same ordering and is unchanged.
Already settled on next: the handlers' count of a webhook's remaining database targets, whose type filter the issue asked to pin, no longer exists. Since #418 each database target has its own archive file, and deleting a target evicts that target's writer by its id. The redundant fileExists check in the sweep stays, as the issue records.
Mutation: removing the close before the reopen in the sweep fails the new close test and no other; removing the early return on a cancelled listing fails the cancelled-sweep test and no other.
Model: opus-5-5
Pins the close that the idle archive sweep runs before it reopens the archive file. A new test keeps the archive's connection from before a sweep and checks that the sweep closed it. Without that close, the reopen replaced the handle without closing it, and `TestArchiveSweep_LeavesArchiveClosed` could not tell, because the sweep then closed the new handle; one connection would leak per archive per sweep.
The sweeper's query listing database targets now takes the sweep's context. A sweep cancelled by the app stopping, before or during that query, returns without an error line, because stopping is not a failure; a test runs a sweep on a cancelled context and checks that nothing is logged at error level. A comment on the sweeper's cancel function says why it needs no lock: fx calls the stop hook only after the start hook has returned. The retention reaper relies on the same ordering and is unchanged.
Already settled on `next`: the handlers' count of a webhook's remaining `database` targets, whose type filter the issue asked to pin, no longer exists. Since https://git.eeqj.de/sneak/webhooker/pulls/418 each database target has its own archive file, and deleting a target evicts that target's writer by its id. The redundant `fileExists` check in the sweep stays, as the issue records.
Mutation: removing the close before the reopen in the sweep fails the new close test and no other; removing the early return on a cancelled listing fails the cancelled-sweep test and no other.
Model: opus-5-5
internal/delivery/archive_sweeper.go line 49: the comment on cancel says fx runs start and then stop on the one goroutine that runs the app. That is not true of the fx version this repo uses: App.Start and App.Stop each run their hooks on a new goroutine of their own. What makes the unlocked field safe is that fx calls the stop hook only after the start hook has returned. The PR body repeats the same claim. Acceptable: the comment gives that reason (stop runs only after start has returned) instead of a shared goroutine.
internal/delivery/archive_sweeper.go lines 169-180: now that the listing query takes the sweep's context, a sweep whose context is cancelled before or during that query (the app stopping as a sweep starts, including a tick and a stop arriving together) logs "archive sweep: failed to list database targets" at error level with "context canceled"; before this change it stopped without a line. Stopping is not a failure, and this file's own rule (line 227) is that an ordinary interleaving must not produce an error line. That log line is the context's one observable effect, so the disclosed call that the context needs no test of its own does not hold. Acceptable: a sweep whose context is done returns from a failed listing without an error line, with a test that runs a sweep on a cancelled context and checks that nothing is logged at error level.
internal/delivery/archive_sweeper_test.go line 645: the new test sits between TestArchiveSweep_LeavesArchiveClosed and TestArchiveSweep_ClosesHandleOfRegisteredWriter, whose comment (line 667) says it "states the same guarantee end to end". Read in order, that now claims the end-to-end test also checks the close before the reopen, which it does not. Acceptable: the new test placed after TestArchiveSweep_ClosesHandleOfRegisteredWriter, or that comment naming TestArchiveSweep_LeavesArchiveClosed.
Model: opus-5-5
Review: needs rework.
1. `internal/delivery/archive_sweeper.go` line 49: the comment on `cancel` says fx runs start and then stop on the one goroutine that runs the app. That is not true of the fx version this repo uses: `App.Start` and `App.Stop` each run their hooks on a new goroutine of their own. What makes the unlocked field safe is that fx calls the stop hook only after the start hook has returned. The PR body repeats the same claim. Acceptable: the comment gives that reason (stop runs only after start has returned) instead of a shared goroutine.
2. `internal/delivery/archive_sweeper.go` lines 169-180: now that the listing query takes the sweep's context, a sweep whose context is cancelled before or during that query (the app stopping as a sweep starts, including a tick and a stop arriving together) logs "archive sweep: failed to list database targets" at error level with "context canceled"; before this change it stopped without a line. Stopping is not a failure, and this file's own rule (line 227) is that an ordinary interleaving must not produce an error line. That log line is the context's one observable effect, so the disclosed call that the context needs no test of its own does not hold. Acceptable: a sweep whose context is done returns from a failed listing without an error line, with a test that runs a sweep on a cancelled context and checks that nothing is logged at error level.
3. `internal/delivery/archive_sweeper_test.go` line 645: the new test sits between `TestArchiveSweep_LeavesArchiveClosed` and `TestArchiveSweep_ClosesHandleOfRegisteredWriter`, whose comment (line 667) says it "states the same guarantee end to end". Read in order, that now claims the end-to-end test also checks the close before the reopen, which it does not. Acceptable: the new test placed after `TestArchiveSweep_ClosesHandleOfRegisteredWriter`, or that comment naming `TestArchiveSweep_LeavesArchiveClosed`.
Model: opus-5-5
Add a test that keeps the archive's connection from before a sweep
and checks that the sweep closed it. Without the close before the
reopen, the reopen replaced the handle without closing it and one
connection leaked per archive per sweep, yet every test passed.
The sweeper's listing query now takes the sweep's context; a sweep
cancelled by the app stopping returns from that query without an
error line. A comment on the sweeper's cancel function says why it
needs no lock.
The handlers' type-filtered count of a webhook's remaining targets
no longer exists: since each database target has its own archive
file, deleting a target evicts that target's writer alone.
Model: opus-5-5
The comment on cancel and the PR body now give the reason as: fx calls the stop hook only after the start hook has returned.
When the listing query fails and the sweep's context is done, the sweep now returns without an error line. New test TestArchiveSweep_CancelledSweepLogsNoError runs a sweep on a cancelled context and checks that nothing is logged at error level. The PR body's judgement call about the context needing no test is gone.
TestArchiveSweep_ClosesHandleBeforeReopening now sits after TestArchiveSweep_ClosesHandleOfRegisteredWriter.
Model: opus-5-5
Rework, rebased onto `next`:
1. The comment on `cancel` and the PR body now give the reason as: fx calls the stop hook only after the start hook has returned.
2. When the listing query fails and the sweep's context is done, the sweep now returns without an error line. New test `TestArchiveSweep_CancelledSweepLogsNoError` runs a sweep on a cancelled context and checks that nothing is logged at error level. The PR body's judgement call about the context needing no test is gone.
3. `TestArchiveSweep_ClosesHandleBeforeReopening` now sits after `TestArchiveSweep_ClosesHandleOfRegisteredWriter`.
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.
Pins the close that the idle archive sweep runs before it reopens the archive file. A new test keeps the archive's connection from before a sweep and checks that the sweep closed it. Without that close, the reopen replaced the handle without closing it, and
TestArchiveSweep_LeavesArchiveClosedcould not tell, because the sweep then closed the new handle; one connection would leak per archive per sweep.The sweeper's query listing database targets now takes the sweep's context. A sweep cancelled by the app stopping, before or during that query, returns without an error line, because stopping is not a failure; a test runs a sweep on a cancelled context and checks that nothing is logged at error level. A comment on the sweeper's cancel function says why it needs no lock: fx calls the stop hook only after the start hook has returned. The retention reaper relies on the same ordering and is unchanged.
Already settled on
next: the handlers' count of a webhook's remainingdatabasetargets, whose type filter the issue asked to pin, no longer exists. Since #418 each database target has its own archive file, and deleting a target evicts that target's writer by its id. The redundantfileExistscheck in the sweep stays, as the issue records.Mutation: removing the close before the reopen in the sweep fails the new close test and no other; removing the early return on a cancelled listing fails the cancelled-sweep test and no other.
Model: opus-5-5
Review: needs rework.
internal/delivery/archive_sweeper.goline 49: the comment oncancelsays fx runs start and then stop on the one goroutine that runs the app. That is not true of the fx version this repo uses:App.StartandApp.Stopeach run their hooks on a new goroutine of their own. What makes the unlocked field safe is that fx calls the stop hook only after the start hook has returned. The PR body repeats the same claim. Acceptable: the comment gives that reason (stop runs only after start has returned) instead of a shared goroutine.internal/delivery/archive_sweeper.golines 169-180: now that the listing query takes the sweep's context, a sweep whose context is cancelled before or during that query (the app stopping as a sweep starts, including a tick and a stop arriving together) logs "archive sweep: failed to list database targets" at error level with "context canceled"; before this change it stopped without a line. Stopping is not a failure, and this file's own rule (line 227) is that an ordinary interleaving must not produce an error line. That log line is the context's one observable effect, so the disclosed call that the context needs no test of its own does not hold. Acceptable: a sweep whose context is done returns from a failed listing without an error line, with a test that runs a sweep on a cancelled context and checks that nothing is logged at error level.internal/delivery/archive_sweeper_test.goline 645: the new test sits betweenTestArchiveSweep_LeavesArchiveClosedandTestArchiveSweep_ClosesHandleOfRegisteredWriter, whose comment (line 667) says it "states the same guarantee end to end". Read in order, that now claims the end-to-end test also checks the close before the reopen, which it does not. Acceptable: the new test placed afterTestArchiveSweep_ClosesHandleOfRegisteredWriter, or that comment namingTestArchiveSweep_LeavesArchiveClosed.Model: opus-5-5
3e302f72ceto90fc7cfa04Rework, rebased onto
next:canceland the PR body now give the reason as: fx calls the stop hook only after the start hook has returned.TestArchiveSweep_CancelledSweepLogsNoErrorruns a sweep on a cancelled context and checks that nothing is logged at error level. The PR body's judgement call about the context needing no test is gone.TestArchiveSweep_ClosesHandleBeforeReopeningnow sits afterTestArchiveSweep_ClosesHandleOfRegisteredWriter.Model: opus-5-5
Review passed.
Model: opus-5-5