feat(server): redesign registration, isolation, and maintenance - #194
Conversation
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThe server was refactored around centralized configuration, isolated task execution, APScheduler maintenance jobs, separated authentication storage, expanded registration profiles, updated Docker deployment controls, and server-local test commands with corresponding documentation and validation coverage. ChangesGREMLIN server modernization
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Client
participant Web as web service
participant Maintenance as maintenance service
participant SQLite
participant Files as result files
Client->>Web: submit or view task
Web->>SQLite: persist task and user state
Maintenance->>SQLite: find expired terminal tasks
Maintenance->>Files: delete validated result artifacts
Maintenance->>SQLite: mark cleaned tasks as deleted
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Python |  |
Jul 28, 2026 9:17a.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| UnusedCode | 1 medium |
| Documentation | 4 minor |
| ErrorProne | 1 high |
| Security | 10 high |
| Complexity | 6 medium |
🟢 Metrics 216 complexity · 14 duplication
Metric Results Complexity 216 Duplication 14
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
YaoYinYing
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd53614291
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| delete_task_artifacts(task, results_folder) | ||
| task_store.update_task( |
There was a problem hiding this comment.
If a user resubmits the same FASTA from an expired failed or cancelled task while this cleanup pass is running, list_tasks() can hold the old terminal row while the upload route replaces it with a new pending run. This code then deletes the new run's result directory and unconditionally changes its row to a deleted status, because deleted-status updates bypass TaskDatabase.update_task()'s terminal guard. Atomically claim the row only while its status and finished_at still match the expired record before removing artifacts.
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #194 +/- ##
==========================================
+ Coverage 73.55% 74.14% +0.58%
==========================================
Files 122 122
Lines 15221 15592 +371
==========================================
+ Hits 11196 11560 +364
- Misses 4025 4032 +7 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/tests/test_auth.py (1)
1044-1069: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSplit
test_auth.py— file now exceeds the 1000-line guideline.Content in this file runs past line 1069. This PR keeps adding new test classes/functions (DB-upgrade tests, profile-field tests) to an already-oversized file.
As per coding guidelines,
**/test*.py: "Keep each test file under 1000 lines and split tests by concern when necessary."Consider splitting into concern-focused modules (e.g.
test_auth_profile.py,test_auth_db_migration.py,test_auth_schemas.py) going forward.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/tests/test_auth.py` around lines 1044 - 1069, Split the oversized test_auth.py into concern-focused test modules, moving schema tests such as test_schema_user_response_excludes_password_hash into a suitable schemas module and grouping profile-field and database-upgrade tests similarly. Preserve all test behavior, fixtures, imports, and coverage while keeping each resulting test file under 1000 lines.Source: Coding guidelines
🧹 Nitpick comments (8)
server/pssm_gremlin_server/schemas.py (1)
110-113: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value
affiliationlacks the length constraint applied to its siblings.
full_name/pi_nameare capped at 128 andRegisterRequest.affiliationat 256, but the admin create/update paths accept unbounded affiliation strings.♻️ Proposed constraint
- affiliation: str | None = None + affiliation: str | None = Field(default=None, max_length=256)(apply in both
AdminCreateUserRequestandAdminUpdateUserRequest)Also applies to: 134-137
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/pssm_gremlin_server/schemas.py` around lines 110 - 113, Add a max_length=256 constraint to the affiliation fields in both AdminCreateUserRequest and AdminUpdateUserRequest, matching the existing RegisterRequest.affiliation limit while preserving their optional defaults.server/pssm_gremlin_server/static/js/user-control.js (1)
353-356: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueNormalize empty
affiliationlike the sibling fields.
full_name,position, andpi_namecollapse empty input tonull, butaffiliationsubmits"", persisting an empty string instead of an unset value.♻️ Proposed change
- affiliation: document.getElementById("newAffiliation").value.trim(), + affiliation: document.getElementById("newAffiliation").value.trim() || null,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/pssm_gremlin_server/static/js/user-control.js` around lines 353 - 356, Normalize the affiliation value in the user creation payload alongside full_name and pi_name: trim newAffiliation and convert an empty result to null. Keep non-empty affiliation values unchanged.server/pssm_gremlin_server/pssm_gremlin.py (1)
362-375: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCompat aliases snapshot values at import time.
Rebinding e.g.
pssm_gremlin.run_pssm_gremlin_in_docker(a pattern tests and legacy callers use) has no effect on the runtime path, sincetask_runtime.run_gremlin_taskresolves its own module globals. A module__getattr__delegating totask_runtimewould keep the aliases live.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/pssm_gremlin_server/pssm_gremlin.py` around lines 362 - 375, Replace the import-time compatibility aliases in the web module with a module-level __getattr__ that delegates requested symbols to task_runtime, so rebinding task_runtime attributes is reflected dynamically. Preserve the existing compatibility names, including run_pssm_gremlin_in_docker and run_gremlin_task, and raise AttributeError for unknown names.server/pssm_gremlin_server/routes.py (1)
1172-1179: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winInconsistent clear semantics across profile fields.
positionhonors an explicitnullviamodel_fields_set, whilefull_name,affiliation, andpi_namesilently ignorenull, so admins can clear position but not the other profile fields. Consider usingmodel_fields_setuniformly.♻️ Proposed change
- if req.affiliation is not None: + if "affiliation" in req.model_fields_set: update_fields["affiliation"] = req.affiliation - if req.full_name is not None: + if "full_name" in req.model_fields_set: update_fields["full_name"] = req.full_name if "position" in req.model_fields_set: update_fields["position"] = req.position - if req.pi_name is not None: + if "pi_name" in req.model_fields_set: update_fields["pi_name"] = req.pi_name🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/pssm_gremlin_server/routes.py` around lines 1172 - 1179, Update the profile update logic around the request fields so affiliation, full_name, and pi_name use req.model_fields_set like position, allowing explicitly provided null values to clear those fields while still ignoring omitted fields.server/pssm_gremlin_server/maintenance/tasks/result_cleanup.py (1)
68-92: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueFull table scan per pass.
list_tasks()materializes every task row on each cleanup run. If the task table grows, a filtered query onstatus/finished_at(with an index) would scale better.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/pssm_gremlin_server/maintenance/tasks/result_cleanup.py` around lines 68 - 92, Update cleanup_expired_task_artifacts to use a task_store query that filters terminal statuses and finished_at values at the database level before iterating, rather than materializing every task via list_tasks(). Add or reuse an indexed query method on TaskDatabase, and preserve the existing artifact deletion, status update, and cleaned count behavior.server/pssm_gremlin_server/task_runtime.py (2)
259-285: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSIGINT handler is installed globally and never restored; also unsafe outside the main thread.
signal.signalraisesValueErrorwhen the task executes on a non-main thread (e.g. Celery--pool=threads/gevent), and the handler keeps referencing a dead container after the run completes.♻️ Proposed hardening
+ previous_sigint = None try: - signal.signal(signal.SIGINT, lambda unused_sig, unused_frame: container.kill()) + try: + previous_sigint = signal.signal( + signal.SIGINT, lambda unused_sig, unused_frame: container.kill() + ) + except ValueError: # not running in the main thread + previous_sigint = None for line in container.logs(stream=True): @@ finally: + if previous_sigint is not None: + try: + signal.signal(signal.SIGINT, previous_sigint) + except ValueError: + pass try: container.remove(force=True) except docker.errors.DockerException: pass🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/pssm_gremlin_server/task_runtime.py` around lines 259 - 285, Update the container execution flow around the SIGINT setup to install the handler only when running in the main thread, preserving the existing behavior for interrupting the active container. Capture the previous SIGINT handler and restore it in the finally block after cleanup, ensuring no handler remains bound to the completed container and non-main-thread execution does not call signal.signal.
261-272: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo wall-clock bound on the container run.
container.logs(stream=True)andcontainer.wait()block indefinitely; a hung GREMLIN run occupies a Celery slot forever. Consider await(timeout=...)plus a Celerytime_limit/soft_time_limiton the task so stuck runs are reaped.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/pssm_gremlin_server/task_runtime.py` around lines 261 - 272, Add a wall-clock timeout to the container execution flow around container.logs(stream=True) and container.wait(), ensuring hung runs are terminated and reported rather than blocking indefinitely. Configure appropriate Celery time_limit and soft_time_limit settings on the task, and preserve existing log/stage processing for runs that complete normally.server/tests/test_process_isolation.py (1)
154-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFragile string-splitting to inspect
docker-compose.yml.This test slices the compose file by substring markers (
"x-task-env:"," web:", etc.) instead of parsing YAML. Any reordering, re-indentation, or added comment line indocker-compose.ymlcan silently shift the slice boundaries and make assertions pass/fail for the wrong reason, without the test failing loudly.Consider
yaml.safe_loadand walking the parsed structure (e.g.services.web.environment,x-task-env, service volume lists) for a more robust, format-independent check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/tests/test_process_isolation.py` around lines 154 - 183, Replace the substring-based extraction in test_compose_isolates_worker_auth_and_web_docker_socket with yaml.safe_load and inspect the parsed top-level extension sections and services.web, services.maintenance, and services.worker structures directly. Normalize environment and volume representations as needed, then preserve the existing assertions for secrets, settings, service commands, auth paths, ports, and Docker socket access without relying on ordering or indentation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/pssm_gremlin_server/config.py`:
- Around line 66-67: Update env_csv to treat an explicitly empty environment
value as unset, matching env_bool, env_str, env_int, and env_float. Select
default when the retrieved value is empty before splitting and stripping
entries, while preserving the existing CSV parsing behavior for non-empty
values.
In `@server/pssm_gremlin_server/task_runtime.py`:
- Around line 246-256: Update run_pssm_gremlin_in_docker’s client.containers.run
invocation to pass CONFIG.docker_user as the container user, ensuring the runner
writes mounted output artifacts with the validated non-root identity.
In `@server/run/restart_pssm_flask.sh`:
- Around line 318-324: Replace the cp-based backup in the user-DB backup block
of restart_pssm_flask.sh with SQLite’s consistent backup API, matching the
approach used by migrate_auth_db.py. Ensure the backup captures committed WAL
data and preserves the existing success and warning messages around the
generated _backup destination.
In `@server/tests/test_tasks.py`:
- Around line 351-415: Replace the misspelled completed-deletion status
“deleted:finshed” with “deleted:finished” across deleted_status_from_task(),
db.py, routes.py, task_runtime.py, and pssm_gremlin.py, including route response
fields and JSON status checks. Update all corresponding test expectations and
fixtures, including
test_cleanup_expired_task_artifacts_only_removes_old_terminal_results, while
preserving other deleted statuses such as “deleted:cancel”.
---
Outside diff comments:
In `@server/tests/test_auth.py`:
- Around line 1044-1069: Split the oversized test_auth.py into concern-focused
test modules, moving schema tests such as
test_schema_user_response_excludes_password_hash into a suitable schemas module
and grouping profile-field and database-upgrade tests similarly. Preserve all
test behavior, fixtures, imports, and coverage while keeping each resulting test
file under 1000 lines.
---
Nitpick comments:
In `@server/pssm_gremlin_server/maintenance/tasks/result_cleanup.py`:
- Around line 68-92: Update cleanup_expired_task_artifacts to use a task_store
query that filters terminal statuses and finished_at values at the database
level before iterating, rather than materializing every task via list_tasks().
Add or reuse an indexed query method on TaskDatabase, and preserve the existing
artifact deletion, status update, and cleaned count behavior.
In `@server/pssm_gremlin_server/pssm_gremlin.py`:
- Around line 362-375: Replace the import-time compatibility aliases in the web
module with a module-level __getattr__ that delegates requested symbols to
task_runtime, so rebinding task_runtime attributes is reflected dynamically.
Preserve the existing compatibility names, including run_pssm_gremlin_in_docker
and run_gremlin_task, and raise AttributeError for unknown names.
In `@server/pssm_gremlin_server/routes.py`:
- Around line 1172-1179: Update the profile update logic around the request
fields so affiliation, full_name, and pi_name use req.model_fields_set like
position, allowing explicitly provided null values to clear those fields while
still ignoring omitted fields.
In `@server/pssm_gremlin_server/schemas.py`:
- Around line 110-113: Add a max_length=256 constraint to the affiliation fields
in both AdminCreateUserRequest and AdminUpdateUserRequest, matching the existing
RegisterRequest.affiliation limit while preserving their optional defaults.
In `@server/pssm_gremlin_server/static/js/user-control.js`:
- Around line 353-356: Normalize the affiliation value in the user creation
payload alongside full_name and pi_name: trim newAffiliation and convert an
empty result to null. Keep non-empty affiliation values unchanged.
In `@server/pssm_gremlin_server/task_runtime.py`:
- Around line 259-285: Update the container execution flow around the SIGINT
setup to install the handler only when running in the main thread, preserving
the existing behavior for interrupting the active container. Capture the
previous SIGINT handler and restore it in the finally block after cleanup,
ensuring no handler remains bound to the completed container and non-main-thread
execution does not call signal.signal.
- Around line 261-272: Add a wall-clock timeout to the container execution flow
around container.logs(stream=True) and container.wait(), ensuring hung runs are
terminated and reported rather than blocking indefinitely. Configure appropriate
Celery time_limit and soft_time_limit settings on the task, and preserve
existing log/stage processing for runs that complete normally.
In `@server/tests/test_process_isolation.py`:
- Around line 154-183: Replace the substring-based extraction in
test_compose_isolates_worker_auth_and_web_docker_socket with yaml.safe_load and
inspect the parsed top-level extension sections and services.web,
services.maintenance, and services.worker structures directly. Normalize
environment and volume representations as needed, then preserve the existing
assertions for secrets, settings, service commands, auth paths, ports, and
Docker socket access without relying on ordering or indentation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fa2ec06e-6a52-4a19-a73b-7977a9f734b1
📒 Files selected for processing (47)
.github/workflows/server-test.yml.gitignoreCHANGELOG.mdMakefiledocs/dev-guide/makefile-reference.mddocs/dev-guide/server.mdpyproject.tomlserver/.coveragercserver/.env.exampleserver/Makefileserver/README.mdserver/docker-compose.ymlserver/pssm_gremlin_server/auth.pyserver/pssm_gremlin_server/config.pyserver/pssm_gremlin_server/db.pyserver/pssm_gremlin_server/maintenance/__init__.pyserver/pssm_gremlin_server/maintenance/manager.pyserver/pssm_gremlin_server/maintenance/model.pyserver/pssm_gremlin_server/maintenance/tasks/__init__.pyserver/pssm_gremlin_server/maintenance/tasks/admin_digest.pyserver/pssm_gremlin_server/maintenance/tasks/database_backup.pyserver/pssm_gremlin_server/maintenance/tasks/result_cleanup.pyserver/pssm_gremlin_server/migrate_auth_db.pyserver/pssm_gremlin_server/pssm_gremlin.pyserver/pssm_gremlin_server/routes.pyserver/pssm_gremlin_server/schemas.pyserver/pssm_gremlin_server/static/css/profile.cssserver/pssm_gremlin_server/static/js/profile.jsserver/pssm_gremlin_server/static/js/register.jsserver/pssm_gremlin_server/static/js/user-control.jsserver/pssm_gremlin_server/task_runtime.pyserver/pssm_gremlin_server/templates/profile.htmlserver/pssm_gremlin_server/templates/register.htmlserver/pssm_gremlin_server/templates/user_control.htmlserver/pyproject.tomlserver/run/restart_pssm_flask.shserver/tests/conftest.pyserver/tests/test_admin.pyserver/tests/test_auth.pyserver/tests/test_config.pyserver/tests/test_database_backup.pyserver/tests/test_docker.pyserver/tests/test_maintenance_manager.pyserver/tests/test_process_isolation.pyserver/tests/test_security.pyserver/tests/test_security_advanced.pyserver/tests/test_tasks.py
💤 Files with no reviewable changes (2)
- pyproject.toml
- Makefile
1 participant
Summary
server/Makefileand update the server GitHub Actions workflow accordinglyWhy
The previous server mixed web, Celery, authentication storage, and periodic work in one runtime boundary. It also lacked the required registration metadata, made deployment modes ambiguous, and left periodic cleanup/notification behavior spread across daemon loops. This redesign gives each process only the storage and settings it needs while keeping existing SQLite deployments upgradeable.
Operational impact
restart_pssm_flask.sh migrate-auth-dbusers.sqlite3RESULT_RETENTION_DAYSaccepts fractions such as0.1(2.4 hours); unset or zero disables cleanupBACKUP_DB_CRON,BACKUP_DB_PATH, and optionalMAX_DB_BACKUP${LOG_DIR}/maintenance.logand remains visible in container logsrestart --mode=devbuilds local images;restart --mode=prodpulls configured images and requires UID/GID 1000:1000Validation
make -C server test— 207 passedbash -n server/run/restart_pssm_flask.shgit diff --checkSummary by CodeRabbit