Deleting a project takes its documentation with it (#164) #166

Merged

qwc merged 1 commit from fix/delete-project-removes-storage into main

2026-09-09 14:25:51 +02:00

qwc commented

2026-09-09 14:12:18 +02:00

Owner

Closes #164. Found while investigating #163; independent of PR #165 and mergeable in either order.

What was wrong

handleAdminDeleteProject removed the search index entries and the database row, and stopped there. Nothing in that path touched storage — and Storage had no way to remove a project at all, only a version.

So every project ever deleted has left its entire directory under storage.base_path behind, versions and all. Nothing reads those directories again and nothing reports them, so they only accumulate. Retention cannot reclaim them either, since it walks projects that still exist.

The sharper problem is reuse: a slug deleted and later recreated inherits the old directory, so files from the previous project can be served under the new one wherever a version tag happens to match.

The fix

FilesystemStorage.DeleteProject, called from the delete handler after the row is gone.

That order is deliberate. A project whose row survived while its files vanished would be broken; a directory that resists removal is exactly the state we already had. So a storage failure is logged, not fatal — the delete the user asked for has happened either way.

It runs under the same per-project lock a rename takes. An upload extracting into that directory while it is being removed would leave half a project on disk, which is the very thing this is meant to prevent.

The guard on RemoveAll

ProjectPath joins the base path with the slug and filepath.Join cleans the result, so a slug that is empty or climbs upwards resolves to the base path or above it — and RemoveAll would take every other project with it. Only a direct child of the base path may be removed.

TestDeleteProjectRefusesToEscapeStorage walks "", ".", "..", "../..", "other/nested" and "/", asserting for each that the call is refused and that both a neighbouring project and the storage base path survive.

Tests

TestDeleteProject — a two-version tree goes, a neighbouring project stays.
TestDeleteProjectMissingDirIsSuccess — unlike MoveProject, nothing is lost deleting a project that was never deployed to, and an error there would fail a delete that already happened in the database.
TestAdminDeleteProjectRemovesStorage — drives the real route end to end.

Both new assertions were confirmed to fail against the old code first:

handler_test.go:4341: project documentation left on disk after delete, stat err = <nil>

Notes for review

Directories already leaked are not cleaned up by this. They have no rows pointing at them any more, so there is nothing to enumerate them from. #164 suggests having startup's ReconcileStorage at least report directories with no project; I have not done that here since it is a separate decision about whether to touch data nobody asked us to touch.
No built-in docs changed. There is no how-to covering project deletion and nothing documented the old behaviour, so there is no text to correct — this restores what the UI already implied. Say the word if you would rather have a manage-projects page and I will write one.
Worth a look at your production storage.base_path for directories with no matching project: on the instance in #163 there are at least three, from the deleted projects that caused that whole failure.

🤖 Written with Claude Code

Closes #164. Found while investigating #163; independent of PR #165 and mergeable in either order. ## What was wrong `handleAdminDeleteProject` removed the search index entries and the database row, and stopped there. Nothing in that path touched storage — and `Storage` had no way to remove a project at all, only a version. So every project ever deleted has left its entire directory under `storage.base_path` behind, versions and all. Nothing reads those directories again and nothing reports them, so they only accumulate. Retention cannot reclaim them either, since it walks projects that still exist. The sharper problem is reuse: a slug deleted and later recreated inherits the old directory, so files from the previous project can be served under the new one wherever a version tag happens to match. ## The fix `FilesystemStorage.DeleteProject`, called from the delete handler after the row is gone. **That order is deliberate.** A project whose row survived while its files vanished would be broken; a directory that resists removal is exactly the state we already had. So a storage failure is logged, not fatal — the delete the user asked for has happened either way. It runs under the same per-project lock a rename takes. An upload extracting into that directory while it is being removed would leave half a project on disk, which is the very thing this is meant to prevent. ## The guard on RemoveAll `ProjectPath` joins the base path with the slug and `filepath.Join` cleans the result, so a slug that is empty or climbs upwards resolves to the base path **or above it** — and `RemoveAll` would take every other project with it. Only a direct child of the base path may be removed. `TestDeleteProjectRefusesToEscapeStorage` walks `""`, `"."`, `".."`, `"../.."`, `"other/nested"` and `"/"`, asserting for each that the call is refused and that both a neighbouring project and the storage base path survive. ## Tests - `TestDeleteProject` — a two-version tree goes, a neighbouring project stays. - `TestDeleteProjectMissingDirIsSuccess` — unlike `MoveProject`, nothing is lost deleting a project that was never deployed to, and an error there would fail a delete that already happened in the database. - `TestAdminDeleteProjectRemovesStorage` — drives the real route end to end. Both new assertions were confirmed to fail against the old code first: ``` handler_test.go:4341: project documentation left on disk after delete, stat err = <nil> ``` ## Notes for review - **Directories already leaked are not cleaned up by this.** They have no rows pointing at them any more, so there is nothing to enumerate them from. #164 suggests having startup's `ReconcileStorage` at least *report* directories with no project; I have not done that here since it is a separate decision about whether to touch data nobody asked us to touch. - **No built-in docs changed.** There is no how-to covering project deletion and nothing documented the old behaviour, so there is no text to correct — this restores what the UI already implied. Say the word if you would rather have a `manage-projects` page and I will write one. - Worth a look at your production `storage.base_path` for directories with no matching project: on the instance in #163 there are at least three, from the deleted projects that caused that whole failure. 🤖 Written with [Claude Code](https://claude.com/claude-code)

qwc added 1 commit

2026-09-09 14:12:18 +02:00

Deleting a project takes its documentation with it (#164 )

CI / test (pull_request) Successful in 1m29s

Details

CI / build (pull_request) Successful in 51s

Details

CI / docker (pull_request) Has been skipped

Details

fe9322b2b2

Deleting a project removed its row and its search index entries and
stopped there. Nothing in that path touched storage, and the storage
interface had no way to remove a project at all -- only a version. So
every project ever deleted left its whole directory under the storage
base path, with all its versions in it.

Nothing reads those directories again and nothing reports them, so they
only ever accumulate. Retention cannot reclaim them either: it walks
projects that still exist. Worse, a slug deleted and later reused
inherits the old directory, so files from the previous project can be
served under the new one wherever a version tag happens to match.

FilesystemStorage gains DeleteProject, and the delete handler calls it
after the row is gone. That order matters: a project whose row survived
while its files vanished would be broken, whereas a directory that
resists removal is the state we already had -- logged, not fatal, since
the delete the user asked for has happened.

It runs under the same per-project lock a rename takes, so an upload
extracting into the directory cannot race the removal and leave half a
project behind.

RemoveAll on a path built from a slug needs a guard: filepath.Join
cleans its result, so an empty slug or one climbing upwards resolves to
the storage base path or above it and would take every other project
with it. Only a direct child of the base path may be removed, and the
test walks "", ".", "..", "../..", "other/nested" and "/" to prove a
neighbouring project and the base path both survive each one.

Both new assertions were confirmed to fail against the old code.

Assisted-by: Claude Opus 5

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Marcel M. Otte <marcel.otte@mmo.to>

qwc merged commit bc35127c57 into main

2026-09-09 14:25:51 +02:00

qwc deleted branch fix/delete-project-removes-storage

2026-09-09 14:25:51 +02:00

qwc referenced this pull request from a commit

2026-09-09 14:25:53 +02:00

Merge pull request 'Deleting a project takes its documentation with it (#164)' (#166) from fix/delete-project-removes-storage into main