fix(api): Phase 7b.1 — upsert name-collision + /readiness proxy gap#77
Conversation
Phase 7b's upsert_runtime_printer keyed only on Printer.id (the new deterministic UUIDv5 from C1). An existing Phase 7a row with a different id but the same name violated the UNIQUE(name) constraint on the INSERT, crashing the backend during the first hhdocker02 deploy. Fix: when the id-lookup misses, fall back to a name-lookup. If a row with the same name exists under a different id, delete it before the new INSERT. New regression test mimics the production state. Refs #76 Refs #22
Phase 7b H1 added /docs, /openapi.json, /redoc proxying but missed /readiness. Production smoke against labels.strausmann.cloud showed the readiness deep-check returning 404 from the frontend edge. One extra r.Handle keeps the path intact and reaches the backend's existing handler. Refs #76 Refs #22
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses two critical issues identified during production deployment. It resolves a database crash occurring during upgrades due to name collisions in the printer table and fixes a routing gap where the /readiness health check endpoint was inaccessible via the public-facing proxy. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #77 +/- ##
==========================================
+ Coverage 91.93% 91.98% +0.04%
==========================================
Files 70 70
Lines 3038 3055 +17
Branches 259 261 +2
==========================================
+ Hits 2793 2810 +17
Misses 181 181
Partials 64 64
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR addresses two production-deploy gaps found after Phase 7b went live: (1) startup failure when upsert_runtime_printer encounters a legacy printers row with a conflicting name, and (2) missing /readiness routing through the Go frontend reverse proxy.
Changes:
- Add a name-based fallback in
upsert_runtime_printerto delete a legacy printer row when the deterministic UUIDv5 lookup misses butnamecollides. - Add
/readinessto the Go frontend proxy allowlist alongside/docs,/openapi.json, and/redoc. - Add/extend regression tests for both the DB upgrade-path collision and the proxy route coverage.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| frontend/cmd/server/main.go | Proxies /readiness to the backend alongside other FastAPI utility routes. |
| frontend/cmd/server/main_test.go | Extends proxy-mount test coverage to include /readiness. |
| backend/tests/integration/db/test_lifespan_printer_upsert.py | Adds regression test seeding a legacy printer row with same name but different id. |
| backend/app/db/lifespan.py | Implements name-collision fallback behavior during runtime-printer upsert. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a database unique constraint conflict (issue #76) by implementing a cleanup step during the application lifespan to remove legacy printer records that share names with new deterministic UUIDv5 entries. It also introduces a /readiness endpoint to the frontend server and includes a regression test for the database fix. Feedback focuses on a potential IntegrityError during the deletion of printer records if cascading deletes are not configured, which could lead to startup crashes. Additionally, a suggestion was made to use standard SQLAlchemy imports instead of aliases to maintain consistency with the rest of the codebase.
Bot reviews on PR #77 flagged that session.delete(existing_by_name) would crash with FOREIGN KEY violations on any real upgrade scenario (jobs/printer_state/printer_status_cache referencing the old printer_id). The previous fix only worked because the production smoke test ran on a wiped DB with no dependent rows — Finding #1 (HIGH). Replace the delete-then-insert path with non-destructive id migration: cascade-rewrite FK columns in printer_status_cache, printer_state, and jobs, then UPDATE the printer row's PK in-place. Historical print jobs survive the upgrade. SQLite stores UUIDs as 32-char hex strings without dashes; raw SQL bind params use str(uuid).replace('-', '') to match the on-disk format. After raw SQL UPDATEs the ORM identity map is expired so the migrated row is visible on the next session.get() call. Also incorporates two MEDIUM findings: - Finding #2: use app.repositories.printers.get_by_name helper instead of re-implementing the select. - Finding #3: drop the _select alias, use select directly like the rest of lifespan.py. New regression test seeds an old printer row WITH dependent jobs, printer_state, and printer_status_cache rows, then confirms the upsert migrates all FK references to the new deterministic UUIDv5 without losing data. Refs #76 Refs #22
Summary
Two Phase 7b foundation gaps surfaced by the first hhdocker02 production deploy of PR #75 (
784decc).1.
upsert_runtime_printercrashed on upgrade pathWhen the lifespan ran against the existing Phase 7a DB (one Printer row with a random UUID4, same
nameas what the new deterministic UUIDv5 wants to write), the INSERT hitUNIQUE constraint failed: printers.name. Crash-loop until the DB was wiped.Fix: name-fallback lookup when the id-lookup misses. If the same
nameexists under a different id, delete that row before the new INSERT.New regression test
test_upsert_handles_existing_row_with_same_name_different_idseeds a Phase-7a-shaped row and asserts the upsert succeeds with the deterministic id.2.
/readinessreturned 404 from the public edgePR #75 Cluster 3 (H1) proxied
/docs,/openapi.json,/redocthrough the Go frontend — but missed/readiness. The deep-check endpoint was reachable only on the backend port 8000, not vialabels.strausmann.cloud.Fix: one extra
r.Handle(\"/readiness\", prx)next to the existing 3. Test extended to assert all 4 paths preserve through the proxy.Test plan
uv run pytest -q— 677 passed, 3 skipped, 0 failed (was 676 — added one regression test)uv run ruff check . && uv run ruff format --check . && uv run mypy app— cleango test ./...—TestProxyMountsBackendDocRoutesnow exercises 4 sub-paths, all greengo vet ./...— cleanlabels.strausmann.cloud/readinessafter merge + deploy — should return HTTP 200 withstatusin{ready, degraded}Deploy notes
Closes #76
Refs #22