Tech-debt batch 1: delete after commit, queued upload cancel, optional settings, generated admin password (#582 #584 #585 #587 #588)

CHANGELOG.md

@@ -8,10 +8,29 @@ All notable changes to this project are documented in this file. The format is b
 > was still unreleased (GH issue #253). Those entries have been moved to the
 > release they actually shipped in, verified with `git tag --contains`.
 > Versions not listed below still have no entry; consult the commit log.
+>
+> 0.0.28 through 0.0.37 were released before the release workflow cut this
+> file (#572), so their changes are still listed under `[Unreleased]` and will
+> land in the next dated section (#584).
 
 ## [Unreleased]
 
 ### Fixed
+- File storage: `DELETE /api/file_storage/files/{id}` and the bulk delete now drop the stored
+  bytes only after the request transaction commits, so a failed commit no longer leaves a live
+  row with no bytes. A new hourly `file_storage.reap_objects` beat task (needs
+  `background_tasks`) retries objects of soft-deleted rows and removes objects left by uploads
+  whose row never committed, after twice the upload-session TTL (#582).
+- File storage: cancelling an upload still waiting for a free slot no longer uploads it later in
+  the background (#588).
+- Tenants: the organisation settings page and its menu item are registered only when the
+  `settings` module is enabled; without it `/tenants/settings` used to 500 with an
+  `ImportError`. `file_storage` now orders after `background_tasks` when both are installed (#585).
+- Docker: **Behaviour change.** With `SM_USERS_BOOTSTRAP_PASSWORD` unset, the entrypoint now
+  generates a random first-boot admin password and prints it (the compose file already said so),
+  instead of seeding the public `changeme`. An explicit `changeme` is refused when
+  `SM_ENVIRONMENT` is `production` (the image default). `docs/release.md`'s local rehearsal now
+  runs in a disposable worktree instead of ending with `git reset --hard HEAD~1` (#587).
 - Retained logs: a NUL character no longer drops the whole log batch on Postgres.
 - Users: after a self-service password change the same browser kept its page session but
   every `/api/users/me*` call (including "Sign out everywhere") answered 401, because the

README.md

@@ -68,20 +68,19 @@ docker build -t simple-module-python .
 docker run --rm -p 8000:8000 -v simple-module-python-data:/app/data simple-module-python
 ```
 
-**Logging in.** With no `SM_USERS_BOOTSTRAP_*` set, the container seeds
-**`admin@example.com` / `changeme`** — the same pair `.env.example` uses for
-local dev — and says so on every boot that uses it:
+**Logging in.** With no `SM_USERS_BOOTSTRAP_PASSWORD` set, the container seeds
+`admin@example.com` with a randomly generated password. The users module logs it
+once when the first admin is created:
 
 ```
-entrypoint: WARNING - no SM_USERS_BOOTSTRAP_PASSWORD set, so the
-entrypoint: first-boot admin is the public default:
-entrypoint:     admin@example.com / changeme
+first-boot admin created; store this password securely: admin@example.com / <generated password>
 ```
 
-Pass `-e SM_USERS_BOOTSTRAP_EMAIL=… -e SM_USERS_BOOTSTRAP_PASSWORD=…` to seed
-your own instead, and change it before the container is reachable by anyone but
-you. The seed only applies while the users table is empty, so on a reused volume
-the existing password still stands — reset it with
+Read it with `docker logs`. Pass `-e SM_USERS_BOOTSTRAP_EMAIL=… -e SM_USERS_BOOTSTRAP_PASSWORD=…`
+to seed your own instead; an explicit `changeme` is refused because the image runs
+with `SM_ENVIRONMENT=production`. The seed only applies while the users table is
+empty, so on a reused volume the existing password still stands and no credentials
+are logged — reset it with
 `smpy users create-admin --email … --password … --force`.
 
 `make docker-compose-app` (or `docker compose up --build app`) runs the same
@@ -101,7 +100,7 @@ bundle rather than a Vite dev server. Useful overrides:
 |---|---|---|
 | `SM_SECRET_KEY` | generated per start | Persist sessions across restarts |
 | `SM_DATABASE_URL` | `sqlite+aiosqlite:////app/data/app.db` | Point at Postgres |
-| `SM_USERS_BOOTSTRAP_EMAIL` / `_PASSWORD` | `admin@example.com` / `changeme` | Seed a first admin nobody else can guess |
+| `SM_USERS_BOOTSTRAP_EMAIL` / `_PASSWORD` | `admin@example.com` / generated and logged | Seed a first admin with a password you chose |
 | `SM_TRUSTED_PROXY` | unset | Set to `*` behind a reverse proxy, so logs record the visitor's IP and not the proxy's |
 
 **No background tasks.** The image skips installing the Celery module

docker-compose.yml

@@ -14,7 +14,10 @@ services:
       - SM_SECRET_KEY
       # First-boot admin seed, applied only while the users table is empty.
       # Unset, the entrypoint seeds admin@example.com with a generated
-      # password and prints it — see `docker compose logs app`.
+      # password; the users module logs it only when creating the admin.
+      # See `docker compose logs app`.
+      # Generated credentials do not reset existing accounts. An explicit
+      # `changeme` password is refused in production.
       - SM_USERS_BOOTSTRAP_EMAIL
       - SM_USERS_BOOTSTRAP_PASSWORD
       # Set to `*` when this sits behind a reverse proxy that terminates TLS,

docker/entrypoint.sh

@@ -9,7 +9,7 @@
 #      place for one), so any that are still unset get an ephemeral random
 #      value — enough to boot and log in, gone on the next start.
 #   2. Seed an admin, so a bare `docker run` lands on a login you can pass
-#      (admin@example.com / changeme unless SM_USERS_BOOTSTRAP_* say otherwise).
+#      (admin@example.com / a generated password unless overridden).
 #   3. Apply migrations. A fresh SQLite volume has no tables at all, and the
 #      boot-time SM010 check fails the app in production when the DB revision
 #      is behind head.
@@ -32,24 +32,21 @@ if [ -n "$_ephemeral" ]; then
     echo "entrypoint: sessions and any reset/verification links they sign die on restart — set these to persist them." >&2
 fi
 
-# The seed the users module applies only while its table is empty, so this
-# can't overwrite an account on a persistent volume — and without it a fresh
-# container serves a login page nobody holds credentials for. The default is
-# deliberately a well-known one: this image is a starting point you are meant
-# to log straight into. Anything running where that matters should pass both
-# vars, which is why the banner below is loud rather than silent.
+# The users module seeds only while its table is empty. These credentials
+# are for first boot, not a password reset for an existing volume.
 : "${SM_USERS_BOOTSTRAP_EMAIL:=admin@example.com}"
-: "${SM_USERS_BOOTSTRAP_PASSWORD:=changeme}"
+if [ -z "${SM_USERS_BOOTSTRAP_PASSWORD:-}" ]; then
+    SM_USERS_BOOTSTRAP_PASSWORD="$(python -c 'import secrets; print(secrets.token_urlsafe(32))')"
+    export SM_USERS_BOOTSTRAP_PASSWORD_GENERATED=1
+fi
 export SM_USERS_BOOTSTRAP_EMAIL SM_USERS_BOOTSTRAP_PASSWORD
 
 if [ "$SM_USERS_BOOTSTRAP_PASSWORD" = "changeme" ]; then
-    echo "entrypoint: WARNING - no SM_USERS_BOOTSTRAP_PASSWORD set, so the" >&2
-    echo "entrypoint: first-boot admin is the public default:" >&2
-    echo "entrypoint:     $SM_USERS_BOOTSTRAP_EMAIL / changeme" >&2
-    echo "entrypoint: Seeded only while no user exists — an existing install is" >&2
-    echo "entrypoint: left alone. Set SM_USERS_BOOTSTRAP_EMAIL/_PASSWORD before" >&2
-    echo "entrypoint: first boot, or change it later with:" >&2
-    echo "entrypoint:     smpy users create-admin --email ... --password ... --force" >&2
+    if [ "${SM_ENVIRONMENT:-production}" = "production" ]; then
+        echo "entrypoint: refusing SM_USERS_BOOTSTRAP_PASSWORD=changeme in production; set a secure password or leave it unset to generate one." >&2
+        exit 1
+    fi
+    echo "entrypoint: WARNING - SM_USERS_BOOTSTRAP_PASSWORD is the public default; set a secure password before deploying." >&2
 fi
 
 # `upgrade heads` (plural) applies every per-module migration branch;

docs/reference/deployment.md

@@ -35,11 +35,12 @@ make docker-compose-app     # same image via the `app` service in docker-compose
 production secret (`SM_SECRET_KEY`, `SM_USERS_RESET_PASSWORD_TOKEN_SECRET`,
 `SM_USERS_VERIFICATION_TOKEN_SECRET`) with an ephemeral random value so a bare
 `docker run` boots — set them yourself for anything that must survive a
-restart. It also seeds `admin@example.com` / `changeme` (warning printed on
-every boot that uses it) unless `SM_USERS_BOOTSTRAP_EMAIL` / `_PASSWORD` say
-otherwise; that seed only lands while the users table is empty, so it never
-overwrites an existing account. A deployment that anyone else can reach should
-set both vars before first boot — the default password is public. The checklist above still applies: that image is a demo/local
+restart. It also seeds `admin@example.com` with a randomly generated password
+logged by the users module only when the first admin is created, unless
+`SM_USERS_BOOTSTRAP_EMAIL` / `_PASSWORD` say otherwise. Store generated credentials securely and restrict access to logs.
+The seed only lands while the users table is empty, so it never overwrites an
+existing account; credentials generated on later starts do not reset passwords.
+An explicit `changeme` password is refused in production. The checklist above still applies: that image is a demo/local
 target, not a production deployment (SQLite, ephemeral secrets).
 
 That image carries no Celery: the build passes

docs/reference/env-vars.md

@@ -82,6 +82,7 @@ Most users settings (signup, OAuth/OIDC credentials, rate limits; SMTP lives in
 |---|---|---|
 | `SM_USERS_BOOTSTRAP_EMAIL` | unset | Auto-creates an admin if set **and** the users table is empty. |
 | `SM_USERS_BOOTSTRAP_PASSWORD` | unset | Paired with the email above. |
+| `SM_USERS_BOOTSTRAP_PASSWORD_GENERATED` | unset | Internal entrypoint marker (`1`): log generated credentials at WARNING only when the bootstrap admin is created. |
 | `SM_USERS_BOOTSTRAP_USER_EMAIL` | unset | Optional second non-admin seed user (handy in dev). |
 | `SM_USERS_BOOTSTRAP_USER_PASSWORD` | unset | Paired with the user email above. |
 | `SM_USERS_RESET_PASSWORD_TOKEN_SECRET` | dev placeholder | HMAC key for password-reset tokens. **Must** be overridden in production (boot fails otherwise). |

docs/release.md

@@ -134,7 +134,7 @@ Expected wall time: 5–8 minutes.
 
 ### Step 4. CHANGELOG and GitHub Release notes
 
-`finalize` runs `scripts/cut_changelog.py X.Y.Z` before the bump commit: `## [Unreleased]` becomes `## [X.Y.Z] - YYYY-MM-DD`, a fresh empty `## [Unreleased]` is inserted above it and the compare links are updated. Re-running is a no-op once the section exists. Keep `[Unreleased]` current in every user-visible PR; whatever is there at release time is the release's notes.
+`finalize` enables `cut-changelog` on the shared `.github/actions/bump-version` action, which runs `scripts/cut_changelog.py X.Y.Z` before the bump commit: `## [Unreleased]` becomes `## [X.Y.Z] — YYYY-MM-DD`, a fresh empty `## [Unreleased]` is inserted above it and the compare links are updated. Re-running is a no-op once the section exists. Keep `[Unreleased]` current in every user-visible PR; whatever is there at release time is the release's notes.
 
 The GitHub Release is created with `--notes-file` from that dated section (`scripts/cut_changelog.py X.Y.Z --notes`), falling back to `--generate-notes` only when the section is empty.
 
@@ -160,16 +160,24 @@ A test (`framework/cli/tests/test_template_requires_framework.py`) pins that the
 
 ## Local pre-flight (optional but recommended)
 
-Before running the workflow, you can rehearse the whole pipeline offline. Nothing leaves your machine:
+Before running the workflow, rehearse in a disposable worktree. Nothing is published.
+The worktree starts at `HEAD`; uncommitted changes in your original checkout are
+not included and remain untouched. Run the following from the repository root:
 
 ```bash
+# 0. Isolate version edits and build artifacts from your working checkout
+release_checkout="$PWD"
+release_rehearsal=$(mktemp -d "${TMPDIR:-/tmp}/smpy-release.XXXXXX")
+git worktree add --detach "$release_rehearsal/worktree" HEAD
+cd "$release_rehearsal/worktree"
+
 # 1. Check every workspace package is currently at 0.0.1 (or your expected base)
 uv run python scripts/bump_version.py 0.0.1 --check
 
 # 2. Dry-run the bump (writes nothing, shows what would change)
 uv run python scripts/bump_version.py 0.0.2 --dry-run
 
-# 3. Actually bump (commit on a throwaway branch if you want to keep it)
+# 3. Actually bump inside the disposable worktree
 uv run python scripts/bump_version.py 0.0.2
 npm install --package-lock-only
 
@@ -186,11 +194,14 @@ mkdir -p dist-npm && for p in packages/*/; do npm pack "$p" --pack-destination d
 ls dist-py/ | wc -l        # expect 30 (a wheel + sdist for each of the 15 buildable packages; only 13 are later published)
 ls dist-npm/ | wc -l       # expect 3
 
-# 7. Revert (if you don't actually want to release)
-git reset --hard HEAD~1
+# 7. Discard only the rehearsal worktree, including its version edits
+cd "$release_checkout"
+git worktree remove --force "$release_rehearsal/worktree"
+rmdir "$release_rehearsal"
 ```
 
 If step 5 fails for any package, the workflow will fail the same way — fix it before dispatching.
+Run step 7 even if an earlier step fails; it removes only the disposable worktree.
 
 ---
 

modules/file_storage/file_storage/backends/filesystem.py

@@ -2,9 +2,12 @@
 
 from __future__ import annotations
 
+import asyncio
 import contextlib
+import os
 import uuid
 from collections.abc import AsyncIterator
+from datetime import datetime
 from pathlib import Path
 
 import aiofiles
@@ -82,6 +85,29 @@ async def delete(self, key: str) -> None:
         path = self._resolve(key)
         path.unlink(missing_ok=True)
 
+    async def list_stale_objects(self, cutoff: datetime) -> AsyncIterator[str]:
+        """Yield keys last modified before the reconciliation cutoff."""
+        keys = await asyncio.to_thread(self._collect_stale_objects, cutoff.timestamp())
+        for key in keys:
+            yield key
+
+    def _collect_stale_objects(self, cutoff_ts: float) -> list[str]:
+        keys = []
+        for directory, dirs, files in os.walk(self.root):
+            if Path(directory) == self.root:
+                dirs[:] = [name for name in dirs if name != constants.FS_MULTIPART_DIR]
+            for name in files:
+                path = Path(directory) / name
+                relative = path.relative_to(self.root)
+                # Preserve the previous shard/key-directory/file glob depth.
+                if len(relative.parts) < 3:
+                    continue
+                # Another delete (including this sweep) may have removed a variant.
+                with contextlib.suppress(FileNotFoundError):
+                    if path.is_file() and path.stat().st_mtime < cutoff_ts:
+                        keys.append(relative.as_posix().split("/", 1)[1])
+        return keys
+
     async def exists(self, key: str) -> bool:
         return self._resolve(key).exists()
 

modules/file_storage/file_storage/backends/s3.py

@@ -8,6 +8,7 @@
 from __future__ import annotations
 
 from collections.abc import AsyncGenerator, AsyncIterator
+from datetime import datetime
 from tempfile import SpooledTemporaryFile
 from typing import Any
 
@@ -129,6 +130,15 @@ async def delete(self, key: str) -> None:
         except Exception as exc:
             raise StorageBackendError(f"S3 delete_object failed: {exc}") from exc
 
+    async def list_stale_objects(self, cutoff: datetime) -> AsyncIterator[str]:
+        """Yield keys last modified before the reconciliation cutoff."""
+        async with self._client() as client:
+            pages = client.get_paginator("list_objects_v2")
+            async for page in pages.paginate(Bucket=self.bucket):
+                for obj in page.get("Contents", []):
+                    if obj["LastModified"] < cutoff:
+                        yield obj["Key"]
+
     async def exists(self, key: str) -> bool:
         try:
             async with self._client() as client:

modules/file_storage/file_storage/constants.py

@@ -9,6 +9,7 @@
 
 from __future__ import annotations
 
+import re
 from typing import Final
 
 # ── Module identity ──────────────────────────────────────────────────
@@ -21,6 +22,7 @@
 MODULE_PASCAL: Final = "FileStorage"
 MODULE_DISPLAY_NAME: Final = "Files"
 _MODULE_SETTINGS: Final = "Settings"
+_MODULE_BACKGROUND_TASKS: Final = "BackgroundTasks"
 
 # ── Configuration ────────────────────────────────────────────────────
 ENV_PREFIX: Final = "SM_FILE_STORAGE_"
@@ -121,6 +123,7 @@ class UploadStatus:
 class Task:
     """Celery task names (``tasks.py``; only loaded when background_tasks is installed)."""
 
+    REAP_OBJECTS: Final = "file_storage.reap_objects"
     REAP_STALE_UPLOADS: Final = "file_storage.reap_stale_uploads"
     COMPUTE_CHECKSUM: Final = "file_storage.compute_checksum"
 
@@ -244,3 +247,7 @@ class I18nKey:
 PUBLIC_CORS_METHODS: Final = ("GET", "HEAD", "OPTIONS")
 PUBLIC_CORS_ALLOW_HEADERS: Final = ("Range", "If-Range", "If-None-Match")
 PUBLIC_CORS_EXPOSE_HEADERS: Final = ("Content-Range", "Accept-Ranges", "ETag", "Content-Length")
+
+# Keys this module writes: ``{tenant}/YYYY/MM/DD/{uuid hex}[.ext]``. Reapers only
+# ever touch objects shaped like this, never foreign ones in a shared store.
+OWN_KEY: Final = re.compile(r"^[^/]+/\d{4}/\d{2}/\d{2}/[0-9a-f]{32}(\.[^/]*)?$")

modules/file_storage/file_storage/deletes.py

@@ -40,7 +40,7 @@ async def get(
         ) -> StoredFile: ...
 
     async def delete_many(
-        self, file_ids: Sequence[uuid.UUID], *, owner: str | None = None
+        self, file_ids: Sequence[uuid.UUID], *, owner: str | None = None, drop_object: bool = True
     ) -> list[StoredFile]:
         """Soft-delete every id that still resolves, returning the rows removed.
 
@@ -63,6 +63,8 @@ async def delete_many(
         # One flush for the batch: the DB rows are what the next page load
         # reads, so they must all be marked before any object is dropped.
         await self.db.flush()
+        if not drop_object:
+            return rows
         for row in rows:
             # Every object is dropped independently. The rows are already
             # marked deleted, so letting one unreachable object abort the loop
@@ -71,14 +73,7 @@ async def delete_many(
             # no row left pointing at them. A failure here is a janitor's
             # problem, not the caller's.
             try:
-                try:
-                    await self.backend.delete(row.key)
-                finally:
-                    # Variants go even when the original's delete fails.
-                    await thumbnails.delete_variants(self.backend, row.key)
-            except StorageNotFoundError:
-                # Acceptably absent — eg. a previous delete partially succeeded.
-                pass
+                await self.drop_object(row)
             except Exception:
                 _logger.exception(
                     "file_storage.bulk_delete_object_failed key=%s — row is deleted, "