Skip to content

Commit e035237

Browse files
committed
Address code-review findings on the #1443 backup sidecar
- Add AdminBackupWarningTests (website/tests/test_backup_status.py): 4 integration tests hitting /admin/ and /admin/data-health/ through a real superuser/editor request, mirroring AdminLoggingWarningTests for the sibling LOG_TO_FILE feature. The existing tests only proved get_backup_status() returns the right dict in isolation, never that MakeabilityLabAdminSite.each_context actually wires BACKUP_STATUS into the rendered templates or that the superuser gate holds at the HTTP layer. - Fix scripts/test_backup_restore_django.sh's "m2m authors restored in order" assertion: it sorted both sides before comparing, so it would have passed even if the restore lost SortedManyToManyField's sort_value ordering (the two test emails happen to also be alphabetical). Seed the through-table out of alphabetical order and compare unsorted, so the assertion can actually fail. - Note the python3 host dependency in scripts/test_backup_restore.sh's header comment (used by its status.json-parsing helpers, unlike the rest of the script which runs entirely inside containers). - Make the db-backup sidecar's entrypoint loop respond to `docker compose stop`/`down` immediately instead of always eating the full 10s stop grace period: bash only processes a trapped signal between foreground commands, so a SIGTERM arriving during the hourly/retry `sleep` sat unhandled until the sleep finished. Add `trap 'exit 0' TERM INT` and background+wait the sleep so the trap fires right away. Verified live: docker stop went from ~10s to 0.12s. Verified end to end in the 1443 worktree: - `manage.py test website.tests.test_backup_status website.tests.test_version_endpoint --settings=makeabilitylab.settings_test` — 24/24 new/touched tests pass (2 unrelated pre-existing failures in test_logging_config, same root cause noted in the PR: local image predates #1439's concurrent-log-handler dependency). - `scripts/test_backup_restore.sh` — 35/35 pass. - `scripts/test_backup_restore_django.sh` — 15/15 pass, including the reworked ordering assertion. - Brought up the real db-backup sidecar via docker-compose-local-dev.yml and confirmed live: correct status.json written, and `docker stop` returns in 0.12s post-fix. 🤖 Generated with [Claude Code](https://claude.com/claude-code) — Sonnet 5, claude-sonnet-5
1 parent faa8cb6 commit e035237

5 files changed

Lines changed: 115 additions & 6 deletions

File tree

docker-compose-local-dev.yml

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,13 @@ services:
120120
# The backup script, mounted as a directory so edits survive git checkouts.
121121
- ./scripts:/backup-scripts:ro
122122

123-
entrypoint: ["/bin/bash", "-c", "while true; do if bash /backup-scripts/pg_backup.sh; then sleep \"$${BACKUP_POLL_SECONDS}\"; else sleep \"$${BACKUP_RETRY_SECONDS}\"; fi; done"]
123+
# `sleep ... & wait $!` plus the leading `trap`, instead of a plain
124+
# foreground `sleep`, so `docker compose stop`/`down` return promptly:
125+
# bash only acts on a trapped signal between foreground commands, so a
126+
# SIGTERM arriving during a plain `sleep 3600` would sit unhandled for up
127+
# to an hour and this container would always eat the full stop grace
128+
# period before Docker escalates to SIGKILL.
129+
entrypoint: ["/bin/bash", "-c", "trap 'exit 0' TERM INT; while true; do if bash /backup-scripts/pg_backup.sh; then sleep \"$${BACKUP_POLL_SECONDS}\" & wait $!; else sleep \"$${BACKUP_RETRY_SECONDS}\" & wait $!; fi; done"]
124130

125131
depends_on:
126132
db:

docker-compose.yml

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,14 @@ services:
2323
# `docker compose up -d` only recreates a container whose config changed, so a
2424
# loop inside the bind-mounted script would keep running the old code forever
2525
# after a deploy. Re-invoking the script each pass makes logic changes deploy.
26+
#
27+
# The `sleep ... & wait $!` (rather than a plain foreground `sleep`) and the
28+
# leading `trap` are so `docker compose stop`/`down` return promptly: bash
29+
# only acts on a trapped signal between foreground commands, so with a plain
30+
# `sleep 3600` a SIGTERM sent while idle would sit unhandled for up to an
31+
# hour and this container would always eat the full stop grace period before
32+
# Docker escalates to SIGKILL. Backgrounding the sleep and `wait`-ing on it
33+
# makes the trap run immediately instead.
2634
db-backup:
2735
image: "${POSTGRES_IMAGE:-postgres}"
2836
restart: always
@@ -38,7 +46,7 @@ services:
3846
- db-data:/var/lib/postgresql/data
3947
- backup-status:/var/backup-status
4048
- ./scripts:/backup-scripts:ro
41-
entrypoint: ["/bin/bash", "-c", "while true; do if bash /backup-scripts/pg_backup.sh; then sleep \"$${BACKUP_POLL_SECONDS}\"; else sleep \"$${BACKUP_RETRY_SECONDS}\"; fi; done"]
49+
entrypoint: ["/bin/bash", "-c", "trap 'exit 0' TERM INT; while true; do if bash /backup-scripts/pg_backup.sh; then sleep \"$${BACKUP_POLL_SECONDS}\" & wait $!; else sleep \"$${BACKUP_RETRY_SECONDS}\" & wait $!; fi; done"]
4250
depends_on:
4351
- db
4452
website:

scripts/test_backup_restore.sh

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,9 @@
2020
# Usage:
2121
# bash scripts/test_backup_restore.sh
2222
#
23-
# Requires: docker, and the postgres image used by the stack.
23+
# Requires: docker, the postgres image used by the stack, and python3 on the
24+
# host (used to parse status.json in the assertions below — everything
25+
# postgres-specific runs inside a container, but that parsing does not).
2426

2527
set -uo pipefail
2628

scripts/test_backup_restore_django.sh

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,11 @@ proj = Project.objects.create(name='Sidewalk "Quoted" Project', short_name='back
121121
122122
pub = Publication.objects.create(title='Notes on the Analytical Engine',
123123
date=datetime.date(2024, 5, 1))
124-
pub.authors.add(p1, p2) # SortedManyToManyField through-table
124+
# Added out of alphabetical order on purpose: SortedManyToManyField orders by
125+
# insertion (sort_value), not by name, so this is the order a restore must
126+
# reproduce. Adding them already-alphabetical would let a restore that lost
127+
# the sort_value column pass by coincidence.
128+
pub.authors.add(p2, p1) # zhang@example.edu, then jose@example.edu
125129
pub.projects.add(proj)
126130
127131
News.objects.create(title='Lab news with <b>HTML</b> & entities',
@@ -196,9 +200,11 @@ assert_eq "manage.py check passes" "0" "$?"
196200
cat > "$WORK_DIR/verify.py" <<'PY'
197201
from website.models import Person, Publication, News
198202
pub = Publication.objects.get(title='Notes on the Analytical Engine')
203+
# Not sorted: this must reflect SortedManyToManyField's own ordering
204+
# (sort_value), which is what proves the restore preserved it.
199205
authors = list(pub.authors.all().values_list('email', flat=True))
200206
news = News.objects.get(title__startswith='Lab news')
201-
print('AUTHORS=' + ','.join(sorted(a or '' for a in authors)))
207+
print('AUTHORS=' + ','.join(a or '' for a in authors))
202208
print('UNICODE=' + Person.objects.get(email='zhang@example.edu').first_name)
203209
print('APOSTROPHE=' + Person.objects.get(email='pat@example.edu').last_name)
204210
print('PROJECTS=' + str(pub.projects.count()))
@@ -209,7 +215,8 @@ VERIFY_OUT="$(docker run --rm --network "$NET" --user root -v "$REPO_DIR:/code"
209215
python manage.py shell -c "exec(open('/verify.py').read())" 2>&1)"
210216
get() { echo "$VERIFY_OUT" | grep "^$1=" | head -1 | cut -d= -f2-; }
211217

212-
assert_eq "m2m authors restored in order" "jose@example.edu,zhang@example.edu" "$(get AUTHORS)"
218+
assert_eq "m2m authors restored in insertion order (not alphabetical)" \
219+
"zhang@example.edu,jose@example.edu" "$(get AUTHORS)"
213220
assert_eq "unicode field via ORM" "" "$(get UNICODE)"
214221
assert_eq "apostrophe field via ORM" "O'Brien" "$(get APOSTROPHE)"
215222
assert_eq "publication↔project m2m" "1" "$(get PROJECTS)"

website/tests/test_backup_status.py

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,15 +10,25 @@
1010
degrades to "unknown" instead of raising. This code runs inside
1111
``each_context``, so an exception would take the entire admin down — the exact
1212
opposite of what a backup-health feature should do.
13+
14+
``BackupStatusFileTests``/``BackupWarningSuppressionTests``/``FormatBytesTests``
15+
exercise ``get_backup_status()`` directly. ``AdminBackupWarningTests`` below
16+
goes one layer further and proves the wiring itself -- that
17+
``MakeabilityLabAdminSite.each_context`` actually reaches the rendered
18+
``/admin/`` and Data Health pages -- mirroring ``AdminLoggingWarningTests`` in
19+
``test_logging_config.py`` for the sibling ``LOG_TO_FILE`` feature.
1320
"""
1421

1522
import json
1623
import os
1724
import tempfile
1825
from datetime import datetime, timedelta, timezone
1926

27+
from django.contrib.auth import get_user_model
2028
from django.test import SimpleTestCase, override_settings
29+
from django.urls import reverse
2130

31+
from website.tests.base import DatabaseTestCase
2232
from website.utils.backup_status import format_bytes, get_backup_status
2333

2434
NOW = datetime(2026, 8, 7, 12, 0, 0, tzinfo=timezone.utc)
@@ -202,6 +212,82 @@ def test_missing_file_is_quiet_in_local_dev(self):
202212
self.assertFalse(get_backup_status(self.missing, NOW)['should_warn'])
203213

204214

215+
class AdminBackupWarningTests(DatabaseTestCase):
216+
"""
217+
The admin dashboard callout and Data Health panel that surface backup
218+
health (#1443), rendered through a real request rather than calling
219+
``get_backup_status()`` in isolation.
220+
221+
``get_backup_status()`` being correct doesn't prove
222+
``MakeabilityLabAdminSite.each_context`` actually wires ``BACKUP_STATUS``
223+
into the templates, or that the superuser gate in ``each_context`` is
224+
doing its job at the HTTP layer -- that's what these tests are for.
225+
"""
226+
227+
def setUp(self):
228+
super().setUp()
229+
User = get_user_model()
230+
self.superuser = User.objects.create_superuser(
231+
username="backupadmin", email="backupadmin@example.com", password="pw-for-test"
232+
)
233+
self.editor = User.objects.create_user(
234+
username="backupeditor",
235+
email="backupeditor@example.com",
236+
password="pw-for-test",
237+
is_staff=True,
238+
)
239+
self.tmp = tempfile.TemporaryDirectory()
240+
self.addCleanup(self.tmp.cleanup)
241+
self.status_file = os.path.join(self.tmp.name, 'status.json')
242+
243+
def _write_status(self, **overrides):
244+
with open(self.status_file, 'w', encoding='utf-8') as handle:
245+
json.dump(_status_payload(**overrides), handle)
246+
return self.status_file
247+
248+
def test_warning_shown_to_superuser_when_backup_unhealthy(self):
249+
self._write_status(last_attempt_ok=False,
250+
error='pg_dump failed (exit 1): connection refused')
251+
with override_settings(BACKUP_STATUS_FILE=self.status_file):
252+
self.client.force_login(self.superuser)
253+
response = self.client.get("/admin/")
254+
self.assertEqual(response.status_code, 200)
255+
self.assertContains(response, "database backups are not healthy")
256+
self.assertContains(response, "connection refused")
257+
258+
def test_no_warning_when_backup_healthy(self):
259+
self._write_status()
260+
with override_settings(BACKUP_STATUS_FILE=self.status_file):
261+
self.client.force_login(self.superuser)
262+
response = self.client.get("/admin/")
263+
self.assertEqual(response.status_code, 200)
264+
self.assertNotContains(response, "database backups are not healthy")
265+
266+
def test_warning_hidden_from_non_superusers(self):
267+
"""Only the maintainer can act on a backup failure, so don't alarm editors."""
268+
self._write_status(last_attempt_ok=False, error='connection refused')
269+
with override_settings(BACKUP_STATUS_FILE=self.status_file):
270+
self.client.force_login(self.editor)
271+
response = self.client.get("/admin/")
272+
self.assertEqual(response.status_code, 200)
273+
self.assertNotContains(response, "database backups are not healthy")
274+
275+
def test_data_health_panel_always_shown_even_when_healthy(self):
276+
"""
277+
Unlike the ``/admin/`` callout, the Data Health panel is rendered
278+
unconditionally -- "when did it last succeed?" is worth showing even
279+
when nothing is wrong.
280+
"""
281+
self._write_status()
282+
with override_settings(BACKUP_STATUS_FILE=self.status_file):
283+
self.client.force_login(self.superuser)
284+
response = self.client.get(reverse("admin:data_health_dashboard"))
285+
self.assertEqual(response.status_code, 200)
286+
self.assertContains(response, "Database backups")
287+
self.assertContains(response, "Healthy")
288+
self.assertContains(response, "makeability-2026-08-07.sql.gz")
289+
290+
205291
class FormatBytesTests(SimpleTestCase):
206292
def test_formats_common_sizes(self):
207293
self.assertEqual(format_bytes(512), '512 B')

0 commit comments

Comments
 (0)