Conversation
| return True | ||
|
|
||
| if self._has_redacted_credential(rendered_task.spec): | ||
| if rendered_task.spec.has_redacted_credentials(): |
There was a problem hiding this comment.
Seems that the children tasks are not checked. When resolving children, should we also check for redacted credentials?
There was a problem hiding this comment.
Fixed by also checking merged children.
There was a problem hiding this comment.
Sorry for reopening it. I wonder if a better fix is to check redaction before merging them into a task, so that such a pattern will be avoided structurally. While the current implementation works, it could fail a well-shaped task just because it is batched with some bad sibling tasks, which is the scheduler's responsibility.
There was a problem hiding this comment.
For safety, I decided to check redaction after resolving the child tasks (mirroring how the parent task is checked after resolution). Fixed by dropping redacted children from the parent task and failing them immediately so that the parent task and the clean children can be dispatched unaffected.
There was a problem hiding this comment.
To ensure atomicity, I think we should unlink all redacted children, mark them failed (with dependent failure cascade), and persist the record of both parent and children within one transaction. We should also mention this behavior in the doc.
There was a problem hiding this comment.
Attempted to fix. Please check.
| redacted_api = self._redact_api() | ||
| if redacted_api is not None: | ||
| data["task"]["spec"]["api"] = redacted_api | ||
| data["task"]["spec"] = self._redact_spec().model_dump( |
There was a problem hiding this comment.
If the user calls record.model_dump(exclude={"task"}), then this line would raise because it assumes the existence of the "task" field. I think we should add a guard
There was a problem hiding this comment.
Good catch. Guard both the "source" and "task" fields.
There was a problem hiding this comment.
It seems that there could be nested exclude like model_dump(exclude={"task": {"spec": {"api"}}}), although I am not sure if we should consider such corner cases.
There was a problem hiding this comment.
Attempted to handle robustly by parsing SerializationInfo.exclude.
There was a problem hiding this comment.
There are other options as well, e.g., include, exclude_unset, and exclude_defaults. If we want to handle it robustly, should we consider them altogether?
There was a problem hiding this comment.
Fixed by revising the processing order: redacting "source" and "task" first before applying handler, so that the arguments to model_dump apply cleanly.
There was a problem hiding this comment.
There is a mode exclude_unset which is not properly addressed, because this way sets them regardless of the option.
There was a problem hiding this comment.
Add and use the copy_preserving_fields_set helper to copy a model while retaining the unset field record across the redact_credentials methods so that TaskRecord._serialize with exclude_unset is correct.
b975365 to
96c4695
Compare
Signed-off-by: Noppanat Wadlom <noppanat.wad@gmail.com>
Signed-off-by: Noppanat Wadlom <noppanat.wad@gmail.com>
Signed-off-by: Noppanat Wadlom <noppanat.wad@gmail.com>
Data profiling, embedding, inference, training (SFT/LoRA/PPO/image classification), and omni specs carry credentials in their data, checkpoint, and related payloads via the shared data mixin and checkpoint request headers, but were persisted and served unredacted. Give each its own redact_credentials/has_redacted_credentials override, chaining super() so subclasses only declare their added fields. Signed-off-by: Noppanat Wadlom <noppanat.wad@gmail.com>
The record serializer replaced task.spec with a fixed mode="python" dump, ignoring the outer call's by_alias/exclude_none/mode. That let the served spec sub-object drift from the rest of the record (e.g. the _upstreamResults alias). Thread the serializer's own SerializationInfo into the redacted-spec dump. Signed-off-by: Noppanat Wadlom <noppanat.wad@gmail.com>
An HTTP output destination's headers are used to deliver results and can carry credentials (e.g. Authorization). Redact them in the base spec so every task type is covered, and have the standalone spec overrides chain super() so the base redaction reaches them. Signed-off-by: Noppanat Wadlom <noppanat.wad@gmail.com>
Dispatch checked only the parent spec for a redacted credential, so a merged child whose credential was redacted at persist time would ship [REDACTED] to the worker after a restart. Check each resolved child and fail the task with credential_not_retained, naming the child. Signed-off-by: Noppanat Wadlom <noppanat.wad@gmail.com>
The record serializer assumed source and task.spec were always present,
so model_dump(exclude={"task"}) raised and exclude={"source"} re-added
the field. Redact each only when the handler output still contains it.
Signed-off-by: Noppanat Wadlom <noppanat.wad@gmail.com>
ModelSpecStrict/Template did not chain super() in redact_credentials or has_redacted_credentials, so model-based specs (embedding, inference, training, serve, omni) skipped the base output-destination header redaction. Chain super() and add a test asserting every spec's redaction reaches the base. Signed-off-by: Noppanat Wadlom <noppanat.wad@gmail.com>
The record serializer replaces the redacted spec wholesale, so a nested
exclude like exclude={"task": {"spec": {"api"}}} re-added the excluded
field. Descend the caller's exclude selector to the spec and forward it
to the redacted-spec dump.
Signed-off-by: Noppanat Wadlom <noppanat.wad@gmail.com>
The record serializer dumped the spec separately and spliced it back, which meant hand-threading each serialization option. Instead redact a copy of the record and let the default handler walk it, so by_alias, exclude/include, exclude_unset, exclude_defaults, and exclude_none all apply to the redacted output natively. Redaction stays scoped to TaskRecord, so the live task and worker dispatch still see real credentials. Drops the manual exclude-descend and IncEx cast. Signed-off-by: Noppanat Wadlom <noppanat.wad@gmail.com>
Dispatch failed the whole task when any merged child had a credential redacted at persist time, sinking well-shaped siblings for how the scheduler batched them. Fail just the offending child during merge resolution and dispatch the parent with its remaining valid children, via a new drop_merged_child that unlinks one child without requeuing. Gating at dispatch rather than merge time also catches a credential that only surfaces after stage-reference resolution. Signed-off-by: Noppanat Wadlom <noppanat.wad@gmail.com>
…ow atomic Merged batches can span workflows (merge keys are spec-based), but plan_merge committed all siblings under the parent's workflow, drifting other workflows' status sets; and failing a redacted merged child unlinked the parent and failed the child in separate transactions. Persist each workflow's records per workflow, and when failing part of a batch commit the failed children (with their dependent cascade) before the parent's unlink — children-first — so a crash can only leave a child failed while the parent still lists it, reconciled on the parent's next dispatch. Signed-off-by: Noppanat Wadlom <noppanat.wad@gmail.com>
The per-class redact_credentials overrides rebuilt the spec with
model_copy(update={...}) on optional fields, and pydantic unions the updated
keys into __pydantic_fields_set__ — so fields the caller never set (data,
inference, checkpoint, nested model config) surfaced under exclude_unset. Add a
redacted_copy helper that restores the original fields-set after the copy
(redaction only rewrites values of existing fields) and use it at every
redaction site.
Signed-off-by: Noppanat Wadlom <noppanat.wad@gmail.com>
The helper performs no redaction — it does model_copy(update=...) while keeping the original fields-set — so name it for that behavior. Also document why object.__setattr__ is needed to write __pydantic_fields_set__. Signed-off-by: Noppanat Wadlom <noppanat.wad@gmail.com>
The helper is a generic pydantic utility with no redaction logic, so move it from tasks/specs/common to shared/utils/pydantic_utils. Also use it for the copies in TaskRecord._serialize, so an optional field later added to that update can't leak under exclude_unset. Signed-off-by: Noppanat Wadlom <noppanat.wad@gmail.com>
2 participants
Purpose
Workflow task records are persisted and exposed through server APIs after submission and restart. PR #137 redacts credentials for
raw_yamland API tasks, but every other credential-bearing task spec also carries secrets, including data retrieval, data profiling, RAG, embedding, inference, training, model-backed/serve, omni, and SSH tasks.This PR extends credential redaction across those task specs while preserving the in-memory task used for execution. It also prevents a task whose credential was redacted during persistence from being resumed after a server restart.
Changes
TaskSpecStrictBaseandTaskSpecTemplateBaseredact the output-destination delivery headers that every task shares; credential-bearing specs overrideredact_credentials()/has_redacted_credentials()for their own fields and chainsuper(), so a subclass only declares the fields it adds.src/shared/utils/redact.pyprovides recursive key-based redaction and credential-marker detection for nested mappings and lists. Existing raw YAML and API redaction use the same helpers.TaskRecordserializes a credential-redacted copy of the record and lets the default handler walk it, so the live task is untouched — execution and worker dispatch still see real credentials — while every dump option (by_alias,exclude/include,exclude_unset,exclude_defaults,exclude_none) applies to the redacted output natively.docs/SERVICE_RESTARTS.mdto describe the credential-retention behavior.Design
Redaction is owned by each task spec because credential locations are task-specific and the base implementation should not make assumptions about arbitrary task data. The base handles only the output destination that every task shares; everything else is declared by the spec that owns it, and overrides chain
super()so shared coverage is never dropped. Shared traversal and key matching live insrc/shared/utils/redact.pyto keep implementations consistent and avoid mutating the execution object.The serialized record caches the redacted spec, just as the source redaction is cached. Credential retention is a dispatchability property, so it is gated at dispatch rather than at merge time;
plan_mergestays purely about merge compatibility. Restart detection is intentionally marker-aware and task-specific, preventing a literal[REDACTED]value in unrelated task data from blocking recovery.Test Plan
Test Result
uv run pre-commit run --all-files— passed: gitleaks, isort, black, ruff, codespell, mypy, and requirements synchronization.uv run pytest tests/ --ignore=tests/worker/test_mp_executor_cleanup_gpu.py -v— 1980 passed, 23 warnings.A separate unfiltered local test run had one failure in the GPU-only vLLM cleanup test because this environment has zero CUDA devices; the repository’s PR workflow excludes that test.
Pre-submission Checklist
pre-commit run --all-filesand fixed any issues.uv run pytest tests/passes locally (the GPU-only cleanup test requires CUDA; the PR-equivalent command above passes).uv sync --all-packages --group ci --frozen).[BREAKING]and described migration steps above.