Repository navigation
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
donovanclarke
left a comment
donovanclarke
left a comment
There was a problem hiding this comment.
approved under standing grant [approve-clean-reviews]
No critical or high findings. The SDK side of the CON-1833 contract lines up with RunPod#6686 (x-runpod-endpoint-id / x-runpod-pod-id + Bearer, exactly one source header, flashTaskCreate selection set within runtimeFields, token placed in RUNPOD_TASK_TOKEN, the four identity vars set on the task pod), with sdk-runtime#4 (/submit accepts only args/kwargs/timeout, the runner never sets RUNPOD_RUNTIME_KIND itself so dev task pods stay on PodTarget, the bootstrap apt-installs manifest systemDependencies and puts the artifact env first on sys.path), and with ai-api#1176's header constants. resolve_mounts returns bindings in spec.mounts order, so both zips in deploy.py are aligned, and resolve_registry_auth(None) short-circuits without an API call. One medium on release sequencing.
SEVERITY - medium
IMPACT - finalizeFlashArtifactUpload { … resourceSpec } plus the resourceSpec.tasks gate make every rp flash deploy, task-less apps included, fail against today's production schema (RunPod#6686 is still open) after the full vendor and upload; worker sentinel calls likewise need ai-api#1176 live.
SOLUTION - release only after #6686 and ai-api#1176 are deployed, or select and require resourceSpec only when the manifest declares tasks so task-less deploys keep working against either backend.
Non-blocking (4)
SEVERITY - low
IMPACT - task env keys with RUNPOD_/FLASH_ prefixes (or PYTHONPATH/PYTHONHOME/LD_PRELOAD) and mounts under /root/.runpod, /runpod-volume, /proc, /sys, /dev pass dev and the full vendor+upload, then fail at finalize with BAD_USER_INPUT (taskPolicy.ts); only endpoints get the local reserved-env check.
SOLUTION - mirror the reserved-env and reserved-path checks for tasks in build_manifest, before build_environment runs.
SEVERITY - low
IMPACT - _deployed_endpoint_input now raises on a user-supplied RUNPOD_API_KEY; worker code that used the previously injected key for wider SDK calls has no explicit opt-in path, and the error does not name the offending key.
SOLUTION - allow explicit user values (the silent injection was the bug), or at least name the key in the error and call out the break in the release notes.
SEVERITY - low
IMPACT - _validate_timeout accepts 0 but flashTaskCreate rejects executionTimeoutSeconds <= 0, so DeployedTaskExecution(..., timeout=0) fails at create; latent today, since no public caller passes a timeout.
SOLUTION - require a positive timeout for deployed executions, or map 0 to None.
SEVERITY - low
IMPACT - _prepare_task_resources and reconcile_endpoints each build their own VolumeResolver, so a volume shared by a task and an endpoint is listed and placement-solved twice per deploy.
SOLUTION - build one resolver in deploy_app and pass it to both.
Unaddressed (0)
Round 1: no prior findings from this reviewer. The four CodeQL / code-quality threads on 8c59d5b are resolved and outdated, and both fixes are in the diff (_TaskTarget base replaces the PodTarget subclassing; asyncio.wait_for replaces the bare await).
Review metadata
review 3 · fable · e49e739
RAN - full diff + 19 changed files read | handles, app, spec, volume, registry, invoker, context, images, secret, events, build, dev, shim, graphql cross-checked at head | RunPod#6686 contract cross-checked (tasks.ts, taskPolicy.ts, taskProvision.ts, taskBootstrap.ts, runtimeFlashAuth.ts, schema diff) | sdk-runtime#4 (bootstrap, task/deployed.py, runner, executor) and ai-api#1176 header constants cross-checked | reviewThreads via GraphQL (4 resolved, outdated) | CI 10/10 success at head
NOT RUN - pytest locally (no checkout) | end-to-end deploy against a #6686 stage
RELIED ON - CI green at e49e739 | bot-thread fixes verified by reading the diff
There was a problem hiding this comment.
🔵 Needs a closer look
The authentication boundary needs coordinated backend integration review, and reserved-field validation still permits partially applied deployments.
1 open issue
What changed in this PR
This PR enables deployed Flash tasks to run by name with JSON arguments and runtime-scoped credentials, removing automatic developer-key injection from deployed endpoints while preserving source-based development execution.
Changes:
- Adds deployed task invocation, public
Taskstubs, and shared execution cleanup. - Resolves task resources during deployment and verifies backend registration.
- Adds runtime identity checks, deployment coverage, and task-aware CLI output.
| File | Description |
|---|---|
| tests/test_cli/test_deploy_command.py | Tests task-only deployment output. |
| tests/test_apps/test_dispatch.py | Supplies deployed identity in dispatch tests. |
| tests/test_apps/test_deployed_tasks.py | Tests credentials, task lifecycle, and registration. |
| tests/test_apps/test_deploy.py | Updates backend response fixtures and formatting. |
| runpod/rp_cli/main.py | Displays all deployed resources. |
| runpod/rp_cli/console.py | Labels registered per-call tasks. |
| runpod/apps/utils/network.py | Selects runtime-scoped authentication headers. |
| runpod/apps/tasks.py | Shares task transport and adds deployed execution. |
| runpod/apps/targets.py | Adds JSON-only deployed task targets. |
| runpod/apps/stubs.py | Introduces the public task client. |
| runpod/apps/handles.py | Adjusts worker self-call routing. |
| runpod/apps/deploy.py | Prepares task resources and validates registration. |
| runpod/apps/context.py | Validates deployed worker identity. |
| runpod/apps/app.py | Routes deployed tasks to named targets. |
| runpod/apps/api.py | Adds task creation and termination operations. |
| runpod/apps/__init__.py | Exports Task from the apps package. |
| runpod/api/mutations/apps.py | Requests registered resource definitions. |
| runpod/api/graphql.py | Supports additional authentication headers. |
| runpod/__init__.py | Exposes the top-level Task import. |
Review effort: Balanced
| if reserved.intersection(resource_env): | ||
| raise InvalidResourceError( | ||
| "production endpoint env contains reserved runtime fields" | ||
| ) |
| } | ||
| runtime = runtime_headers() | ||
| headers.update( | ||
| runtime if runtime is not None else {"Authorization": f"Bearer {_api_key()}"} |
There was a problem hiding this comment.
Inside a deployed worker, runtime_headers() now always replaces the account key, and that covers every SentinelTarget, including the Queue/Api/Task stubs the module docstring pitches for exactly this (runpod.Queue(app="other-app", name="transcribe")). On the ai-api side (#1176, UserAuth), a runtime caller gets a 401 unless runtimeIdentity.FlashEnvironmentId == endpoint.FlashEnvironmentId, and every other app has its own environment. So any stub call to another app from production worker code now fails, where before this PR it worked with the injected RUNPOD_API_KEY.
There's also no way around it: RUNPOD_API_KEY is now reserved so deploy rejects it, and setting runpod.api_key by hand doesn't help because _api_key() is only consulted when runtime_headers() returns None. Is dropping cross-app calls from production intended for CON-1833? If so, the stub docs should say they only work locally or within the same app, and a clear error before the request would beat a 401 from the sentinel. If not, could there be an explicit opt-in, e.g. a user-provided key secret the stubs prefer for cross-app targets?
| ) | ||
|
|
||
| def build_payload(self, fn, spec, args, kwargs): | ||
| return {"args": list(args), "kwargs": kwargs} |
There was a problem hiding this comment.
The same my_task.remote(...) call goes through PodTarget from a laptop or dev session, which cloudpickles the args, but through DeployedTaskTarget in a deployed worker, which only accepts JSON. So a task that takes bytes, a numpy array, a dataclass or a datetime works in rp flash dev and in local tests and fails only once deployed, and for those types the failure is a bare TypeError from json.dumps rather than the "deployed tasks accept only JSON args and kwargs" ValueError. Could dev and local enforce the same JSON check so this shows up before deploy, or could deploy / rp flash dev at least warn? Catching TypeError alongside the JSON check would also keep the error message consistent.
celine-runpod
changed the title
Part of CON-1833.
Deploy and invoke registered Flash tasks using endpoint or pod runtime credentials. Deployment resolves task resources under deployer credentials, and workers invoke tasks by name with JSON arguments. Recursive calls, cancellation cleanup, task-only apps, and per-call deployment output are supported.
Requires RunPod#6686, ai-api#1176, and sdk-runtime#4. Deployed workers require app/environment/resource/build identity; dev retains its current source-based execution.