Skip to content

fix: cap running, retained and caller-tuned FHIR bulk and SMART ingestion jobs - #4199

Open
pardeep-singh wants to merge 1 commit into
maziyarpanahi:masterfrom
pardeep-singh:pardeep/issue3909-bulk-job-bounds
Open

pardeep-singh wants to merge 1 commit into
maziyarpanahi:masterfrom
pardeep-singh:pardeep/issue3909-bulk-job-bounds

Conversation

@pardeep-singh

Copy link
Copy Markdown
Contributor

Description

Fixes #3909. On master, FHIRBulkJobManager.start and SMARTBackendJobManager.start created a task for every request. Nothing capped running jobs, and finished records stayed in _jobs for the life of the process. The request schemas had only lower bounds, and poll_interval_seconds=0 was accepted. Callers could pick unbounded parallelism, buffering and remote polling rates.

This PR adds server-owned admission, retention and tuning bounds to both managers.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Code refactoring
  • Performance improvement
  • Test addition/improvement

Changes Made

  • openmed/service/job_bounds.py (new): BulkJobBounds (running ceiling, retained cap, terminal TTL, retry delay), the typed BulkJobsSaturatedError, and shared admission and pruning helpers. Validates max_running_jobs <= max_retained_jobs.
  • openmed/service/bulk_data.py, openmed/service/smart_backend.py: both managers take optional bounds and an injected clock. start rejects before creating a task. start, get and cancel prune expired terminal records lazily. New ensure_capacity() and bounds. bulk_job_bounds_from_env() reads three OPENMED_SERVICE_BULK_* settings with the existing positive-integer parser.
  • openmed/service/app.py: reads bounds once in create_app, passes them to both managers, and calls ensure_capacity() before the output directory is reserved. Maps the error to 503 bulk_jobs_saturated with Retry-After.
  • openmed/service/schemas.py: upper bounds on max_inflight_downloads (16), max_buffered_resources (1000), request_timeout_seconds (300) and poll_interval_seconds (1 to 300) for the bulk and SMART requests, both pydantic branches.
  • openmed/service/api_compatibility.py: registers bulk_jobs_saturated as a stable error category.
  • docs/api/openapi.json: regenerated with scripts/export_openapi.py.
  • docs/interop/fhir-bulk-data.md, docs/rest-service.md, CHANGELOG.md: settings, limits, error codes and migration notes.
  • tests/unit/service/test_bulk_job_bounds.py (new) and tests/unit/service/test_smart_backend.py (fake manager gains ensure_capacity).

Testing

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have tested this change with different models/inputs

New tests cover, for both managers: the N+1th start rejected with no new task, transport call or directory; a slot freed after a job finishes or is cancelled; TTL expiry under an injected clock; 10,000 synthetic jobs with len(_jobs) never above the cap; running jobs never evicted; an expired id returning the same 404 as an unknown id; every out-of-range tuning value (including poll_interval_seconds of 0, 0.5 and infinity) rejected with 422 and controlled codes; in-range values forwarded unchanged; bounds and env validation. Sentinel URLs, paths, keys, client ids and assertions never appear in rejection bodies, headers or caplog.

Before the fix the new file gave 36 failed, 11 passed (the passing ones are in-range and already-rejected lower-bound cases). After: 47 passed.

PYTHONPATH=$PWD ../.venv/bin/python -m pytest tests/unit/service/test_bulk_job_bounds.py -q
# before: 36 failed, 11 passed; after: 47 passed
PYTHONPATH=$PWD ../.venv/bin/python -m pytest tests/unit/service -q
# before: 933 passed, 5 skipped; after: 980 passed, 5 skipped
PYTHONPATH=$PWD ../.venv/bin/python -m pytest tests/unit/interop/fhir tests/unit/security -q --deselect tests/unit/security/test_vulnerability_scan_gate.py
# 1056 passed
PYTHONPATH=$PWD ../.venv/bin/python -m pytest tests/unit/release/test_changelog.py tests/unit/test_troubleshooting_docs.py tests/unit/service/test_postman_collection.py tests/unit/service/test_rest_client.py -q
# 60 passed
PYTHONPATH=$PWD ../.venv/bin/python scripts/security/check_effect_paths.py --check
# effect_paths_verified
../.venv/bin/ruff check . && ../.venv/bin/ruff format --check .
# All checks passed! / 2886 files already formatted

Documentation

  • I have updated the documentation accordingly
  • I have added docstrings to new functions/classes
  • I have updated the CHANGELOG.md

Code Quality

  • I ran make format, make lint, and make format-check (ran the pinned ruff check . and ruff format --check . directly)
  • For Swift/OpenMedKit changes, I ran make format-swift and make lint-swift
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings

Dependencies

  • I have not added any new dependencies

Notes

  • Settings. OPENMED_SERVICE_BULK_MAX_RUNNING_JOBS (default 4), OPENMED_SERVICE_BULK_MAX_RETAINED_JOBS (256) and OPENMED_SERVICE_BULK_JOB_TTL_SECONDS (3600). Each manager applies them on its own. Invalid values, or a running ceiling above the retained cap, fail at startup.
  • Retention invariant. Running records are never evicted. Admission keeps running jobs below the running ceiling, and that ceiling is at most the retained cap, so a start can always evict a terminal record when it needs room. The oldest terminal records go first.
  • Lazy expiry. No background thread or timer. Records are pruned on start, status, report, manifest, summary and cancel. An expired id raises the same KeyError as an unknown id, so routes return the same 404.
  • Admission before reservation. The routes call ensure_capacity() before BulkStoragePolicy.prepare, so a saturated start leaves no output directory. start() checks again before create_task.
  • Drain (Drain requests, streams, jobs, and accelerator workers without lost or duplicate work #3576). Shutdown still cancels every running job. Bulk cancel_all now cancels records directly, so a concurrent prune cannot cause a KeyError during shutdown.
  • Fixed schema ceilings. The tuning limits are le=/ge= schema bounds, not runtime settings. They show up in the OpenAPI contract and use the existing 422 validation_error path with pydantic codes such as less_than_equal. The details never echo the submitted value.
  • Compatibility. Requests with poll_interval_seconds below 1 or values above the new ceilings now get 422. The compatibility gate does not flag bound changes; the OpenAPI diff is the three new maximum values and the poll minimum raised from 0 to 1. The direct Python SMARTBackendConfig and FHIRBulkJobConfig keep their previous lower bounds, so library tests that poll with an injected sleep still work.
  • Retry-After. Fixed at 30 seconds through BulkJobBounds.retry_after_seconds. It is not an env setting.
  • Out of scope. A remote server's own Retry-After: 0 is still honored as before. Path confinement and egress (Enforce explicit egress policy for remote inference, callbacks, and connectors #3581) and durable recovery or idempotency keys (Deduplicate retried bulk-export and ingestion starts with idempotency keys #3923, Make asynchronous jobs durable with leases, checkpoints, poison quarantine, and replay #3574) are not touched.
  • The Go client test that sends PollIntervalSeconds: 0 checks only client serialization against a mock server, so it still passes. A real server now rejects that value.

Related Issues

Closes #3909
Related to #3576, #3581, #3923, #3574

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cap concurrent, retained and caller-tuned FHIR bulk and SMART ingestion jobs

1 participant