Found and measured during the rework of #256, and deliberately left out of that PR because closing archive writers is a change to the archive lifecycle rather than part of the durability fix.
The delivery engine caches archive writers and never closes them on shutdown. After a clean stop, the archive database's -wal sidecar still holds the data: measured with archive-*.db carrying no schema at all and the -wal holding all 8 rows.
Not a correctness hole today. cp -a of the whole DATA_DIR — which is what the documented backup procedure does — carries the sidecars, so nothing is lost by following the docs. #263 documents the behaviour rather than leaving it silent.
But it makes the archive a three-file artifact where an operator reasonably expects one. Anyone who moves or copies archive-<id>.db on its own — the obvious thing to do with a file named that — gets an empty database and no warning. That is the shape of a foot-gun rather than a bug.
Definition of done
Archive writers are closed in the delivery engine's OnStop hook, so a clean shutdown checkpoints and removes the -wal.
A test asserting no archive-*.db-wal survives a clean shutdown.
The README caveat added by #263 documenting this gap is reverted, since it will no longer be true.
Confirm the close path cannot block or panic if a writer is mid-write when the stop hook fires, and that it respects the existing stop-context budget rather than extending shutdown past it — #134 and #102 set that budget and must not regress.
Confirm a kill -9 still leaves recoverable state — this makes the CLEAN path single-file, it must not make the unclean path worse.
Not milestoned: no data is lost under the documented procedure, and the fix touches shutdown ordering, which is worth doing carefully rather than quickly.
Found and measured during the rework of https://git.eeqj.de/sneak/webhooker/issues/256, and deliberately left out of that PR because closing archive writers is a change to the archive lifecycle rather than part of the durability fix.
The delivery engine caches archive writers and never closes them on shutdown. After a clean stop, the archive database's `-wal` sidecar still holds the data: measured with `archive-*.db` carrying no schema at all and the `-wal` holding all 8 rows.
Not a correctness hole today. `cp -a` of the whole `DATA_DIR` — which is what the documented backup procedure does — carries the sidecars, so nothing is lost by following the docs. https://git.eeqj.de/sneak/webhooker/pulls/263 documents the behaviour rather than leaving it silent.
But it makes the archive a three-file artifact where an operator reasonably expects one. Anyone who moves or copies `archive-<id>.db` on its own — the obvious thing to do with a file named that — gets an empty database and no warning. That is the shape of a foot-gun rather than a bug.
## Definition of done
- Archive writers are closed in the delivery engine's `OnStop` hook, so a clean shutdown checkpoints and removes the `-wal`.
- A test asserting no `archive-*.db-wal` survives a clean shutdown.
- The README caveat added by https://git.eeqj.de/sneak/webhooker/pulls/263 documenting this gap is reverted, since it will no longer be true.
- Confirm the close path cannot block or panic if a writer is mid-write when the stop hook fires, and that it respects the existing stop-context budget rather than extending shutdown past it — https://git.eeqj.de/sneak/webhooker/issues/134 and https://git.eeqj.de/sneak/webhooker/issues/102 set that budget and must not regress.
- Confirm a `kill -9` still leaves recoverable state — this makes the CLEAN path single-file, it must not make the unclean path worse.
Not milestoned: no data is lost under the documented procedure, and the fix touches shutdown ordering, which is worth doing carefully rather than quickly.
Plan. The gap is still on next (3cdab97). The database target's writer registry (internal/delivery/target_database.go) is closed only per webhook, through evict, and never at shutdown. The README's backup section still carries the caveat added by #263.
Fix: at shutdown, after the engine's workers have stopped, close every cached archive writer the way evict already does: under each writer's own mutex, marking it evicted, so a late write is refused rather than reopening the file. archiveWriter.write already refuses an evicted writer, and archiveWriter.close already closes the handle.
Registry ordering: the archive sweeper (internal/delivery/archive_sweeper.go) is a separate fx component with its own stop hook, and it can create registry entries through sweepWriterFor. The shutdown close must not be followed by a sweeper run that opens a fresh writer and leaves it open. Close the registry only once both have stopped, or make the registry refuse new writers after the close. Pick the plainer one and say which on the PR.
Shutdown budget: stay inside the existing stop context (#134, #102). If the engine's wait for its workers times out, the close must still not block past the budget or race a write. The per-writer mutex already serialises a close against an in-progress write; make sure nothing can wait on it longer than one write.
Tests:
After a clean stop that followed an archived delivery, no archive-*.db-wal remains, and the .db alone holds the rows.
A write after the close is refused, not written.
README: revert the archive caveat in "What to back up", since a clean stop now closes archive databases like the others. Keep the part about a killed instance, which still leaves sidecars. kill -9 behaviour is unchanged; confirm it and say nothing new about it.
Model: opus-5-5
Plan. The gap is still on `next` (`3cdab97`). The database target's writer registry (`internal/delivery/target_database.go`) is closed only per webhook, through `evict`, and never at shutdown. The README's backup section still carries the caveat added by https://git.eeqj.de/sneak/webhooker/pulls/263.
- **Fix:** at shutdown, after the engine's workers have stopped, close every cached archive writer the way `evict` already does: under each writer's own mutex, marking it evicted, so a late write is refused rather than reopening the file. `archiveWriter.write` already refuses an evicted writer, and `archiveWriter.close` already closes the handle.
- **Registry ordering:** the archive sweeper (`internal/delivery/archive_sweeper.go`) is a separate fx component with its own stop hook, and it can create registry entries through `sweepWriterFor`. The shutdown close must not be followed by a sweeper run that opens a fresh writer and leaves it open. Close the registry only once both have stopped, or make the registry refuse new writers after the close. Pick the plainer one and say which on the PR.
- **Shutdown budget:** stay inside the existing stop context (https://git.eeqj.de/sneak/webhooker/issues/134, https://git.eeqj.de/sneak/webhooker/issues/102). If the engine's wait for its workers times out, the close must still not block past the budget or race a write. The per-writer mutex already serialises a close against an in-progress write; make sure nothing can wait on it longer than one write.
- **Tests:**
- After a clean stop that followed an archived delivery, no `archive-*.db-wal` remains, and the `.db` alone holds the rows.
- A write after the close is refused, not written.
- **README:** revert the archive caveat in "What to back up", since a clean stop now closes archive databases like the others. Keep the part about a killed instance, which still leaves sidecars. `kill -9` behaviour is unchanged; confirm it and say nothing new about it.
Model: opus-5-5
clawbot
self-assigned this 2026-09-29 04:31:01 +02:00
Implemented in #332: the engine's stop hook now closes every archive writer once its workers have returned, so a clean stop leaves each archive as one file. The README caveat is removed.
Model: opus-5-5
Implemented in https://git.eeqj.de/sneak/webhooker/pulls/332: the engine's stop hook now closes every archive writer once its workers have returned, so a clean stop leaves each archive as one file. The README caveat is removed.
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.
Found and measured during the rework of #256, and deliberately left out of that PR because closing archive writers is a change to the archive lifecycle rather than part of the durability fix.
The delivery engine caches archive writers and never closes them on shutdown. After a clean stop, the archive database's
-walsidecar still holds the data: measured witharchive-*.dbcarrying no schema at all and the-walholding all 8 rows.Not a correctness hole today.
cp -aof the wholeDATA_DIR— which is what the documented backup procedure does — carries the sidecars, so nothing is lost by following the docs. #263 documents the behaviour rather than leaving it silent.But it makes the archive a three-file artifact where an operator reasonably expects one. Anyone who moves or copies
archive-<id>.dbon its own — the obvious thing to do with a file named that — gets an empty database and no warning. That is the shape of a foot-gun rather than a bug.Definition of done
OnStophook, so a clean shutdown checkpoints and removes the-wal.archive-*.db-walsurvives a clean shutdown.kill -9still leaves recoverable state — this makes the CLEAN path single-file, it must not make the unclean path worse.Not milestoned: no data is lost under the documented procedure, and the fix touches shutdown ordering, which is worth doing carefully rather than quickly.
Plan. The gap is still on
next(3cdab97). The database target's writer registry (internal/delivery/target_database.go) is closed only per webhook, throughevict, and never at shutdown. The README's backup section still carries the caveat added by #263.evictalready does: under each writer's own mutex, marking it evicted, so a late write is refused rather than reopening the file.archiveWriter.writealready refuses an evicted writer, andarchiveWriter.closealready closes the handle.internal/delivery/archive_sweeper.go) is a separate fx component with its own stop hook, and it can create registry entries throughsweepWriterFor. The shutdown close must not be followed by a sweeper run that opens a fresh writer and leaves it open. Close the registry only once both have stopped, or make the registry refuse new writers after the close. Pick the plainer one and say which on the PR.archive-*.db-walremains, and the.dbalone holds the rows.kill -9behaviour is unchanged; confirm it and say nothing new about it.Model: opus-5-5
Implemented in #332: the engine's stop hook now closes every archive writer once its workers have returned, so a clean stop leaves each archive as one file. The README caveat is removed.
Model: opus-5-5