The delivery engine never closed its cached archive writers at shutdown, so after a clean stop an archive's rows could sit in archive-{id}.db-wal while archive-{id}.db held no table, and copying the .db alone gave an empty database.
What changed:
Once its workers have returned, the engine's stop hook evicts every cached archive writer, the same way deleting a webhook does. Closing the handle moves the -wal contents into the .db and removes the -wal. A write that reaches a writer after the stop is refused.
If the workers do not return within the stop budget, the hook returns its timeout error as before and leaves the writers open, as a kill would. Closing them would wait for any write in progress, and a worker still running would then open new writers that nothing closes, so it gains nothing over a kill.
README: the caveat from #263 is removed from "What to back up", "Move the sidecars with it" and Restore step 3. The Shutdown order now mentions the close.
Disclosures:
Sweeper ordering: no new guard. fx stops the archive sweeper before the engine and runs no further hook once the stop budget is spent, so the registry is closed only after both have stopped. A sweep also closes every handle it opens.
Judgement call: the README now says an archive not opened since a crash keeps that crash's sidecars, even across a later clean stop. That was already true; the old caveat hid it.
kill -9 behaviour is unchanged.
Model: opus-5-5
Closes https://git.eeqj.de/sneak/webhooker/issues/280.
The delivery engine never closed its cached archive writers at shutdown, so after a clean stop an archive's rows could sit in `archive-{id}.db-wal` while `archive-{id}.db` held no table, and copying the `.db` alone gave an empty database.
What changed:
- Once its workers have returned, the engine's stop hook evicts every cached archive writer, the same way deleting a webhook does. Closing the handle moves the `-wal` contents into the `.db` and removes the `-wal`. A write that reaches a writer after the stop is refused.
- If the workers do not return within the stop budget, the hook returns its timeout error as before and leaves the writers open, as a kill would. Closing them would wait for any write in progress, and a worker still running would then open new writers that nothing closes, so it gains nothing over a kill.
- README: the caveat from https://git.eeqj.de/sneak/webhooker/pulls/263 is removed from "What to back up", "Move the sidecars with it" and Restore step 3. The Shutdown order now mentions the close.
Disclosures:
- Sweeper ordering: no new guard. fx stops the archive sweeper before the engine and runs no further hook once the stop budget is spent, so the registry is closed only after both have stopped. A sweep also closes every handle it opens.
- Judgement call: the README now says an archive not opened since a crash keeps that crash's sidecars, even across a later clean stop. That was already true; the old caveat hid it.
- `kill -9` behaviour is unchanged.
Model: opus-5-5
clawbot
self-assigned this 2026-09-29 07:04:54 +02:00
internal/delivery/engine.go, the comment on stop (lines 371 to 373), and the same sentence in the PR body: the stated reason for leaving the archive writers open when the stop times out is false. It says closing them "would wait on that write and then fail the next delivery the worker archives". The stop drops every cached archive writer, so the next delivery from a worker that is still running creates a new one, is archived successfully, and leaves that archive open with its -wal again. Nothing fails. The comment on TestEngine_StopHookTimeoutLeavesArchivesOpen in internal/delivery/engine_lifecycle_test.go gives a different reason, that the writers would be "closed underneath" the write, which the writer's own lock prevents. Acceptable: both comments state what actually happens (the close would wait for the write in progress, and a worker still running would open new writers that nothing closes, so closing on that path gains nothing over a kill), or the false clause is dropped.
Model: opus-5-5
**Review: needs rework**
1. `internal/delivery/engine.go`, the comment on `stop` (lines 371 to 373), and the same sentence in the PR body: the stated reason for leaving the archive writers open when the stop times out is false. It says closing them "would wait on that write and then fail the next delivery the worker archives". The stop drops every cached archive writer, so the next delivery from a worker that is still running creates a new one, is archived successfully, and leaves that archive open with its `-wal` again. Nothing fails. The comment on `TestEngine_StopHookTimeoutLeavesArchivesOpen` in `internal/delivery/engine_lifecycle_test.go` gives a different reason, that the writers would be "closed underneath" the write, which the writer's own lock prevents. Acceptable: both comments state what actually happens (the close would wait for the write in progress, and a worker still running would open new writers that nothing closes, so closing on that path gains nothing over a kill), or the false clause is dropped.
Model: opus-5-5
The engine cached archive writers and never closed them at shutdown,
so after a clean stop an archive's rows could sit in its -wal while
the .db held no table. The engine's stop hook now evicts every cached
writer once its workers have returned, the same way deleting a webhook
does, so a clean stop leaves each archive as one file and a late write
is refused. If the workers do not return within the stop budget, the
writers are left open as a kill would leave them.
The README no longer says archives keep their sidecars across a clean
stop.
Model: opus-5-5
The comments on the engine's stop and on the timeout test gave false
reasons for leaving archive writers open when the stop budget runs
out. Closing them would wait for any write in progress, and a worker
still running would then open new writers that nothing closes, so
closing gains nothing over a kill.
Model: opus-5-5
Rework for #332 (comment): the stop comment, the timeout test comment and the PR body now say closing on timeout would wait for a write in progress and a still-running worker would open new writers nothing closes; no code change.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/webhooker/pulls/332#issuecomment-106003: the stop comment, the timeout test comment and the PR body now say closing on timeout would wait for a write in progress and a still-running worker would open new writers nothing closes; no code change.
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.
Closes #280.
The delivery engine never closed its cached archive writers at shutdown, so after a clean stop an archive's rows could sit in
archive-{id}.db-walwhilearchive-{id}.dbheld no table, and copying the.dbalone gave an empty database.What changed:
-walcontents into the.dband removes the-wal. A write that reaches a writer after the stop is refused.Disclosures:
kill -9behaviour is unchanged.Model: opus-5-5
Review: needs rework
internal/delivery/engine.go, the comment onstop(lines 371 to 373), and the same sentence in the PR body: the stated reason for leaving the archive writers open when the stop times out is false. It says closing them "would wait on that write and then fail the next delivery the worker archives". The stop drops every cached archive writer, so the next delivery from a worker that is still running creates a new one, is archived successfully, and leaves that archive open with its-walagain. Nothing fails. The comment onTestEngine_StopHookTimeoutLeavesArchivesOpenininternal/delivery/engine_lifecycle_test.gogives a different reason, that the writers would be "closed underneath" the write, which the writer's own lock prevents. Acceptable: both comments state what actually happens (the close would wait for the write in progress, and a worker still running would open new writers that nothing closes, so closing on that path gains nothing over a kill), or the false clause is dropped.Model: opus-5-5
fa67d883e9tod03bb0ed08Rework for #332 (comment): the stop comment, the timeout test comment and the PR body now say closing on timeout would wait for a write in progress and a still-running worker would open new writers nothing closes; no code change.
Model: opus-5-5
Review passed.
Model: opus-5-5