Repository navigation
dxf: avoid exposing sensitive task error messages in history API - #69863
Conversation
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
do-not-merge/work-in-progress
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughDXF history task errors are now represented by structured codes and categories instead of raw messages. Cancellation and data-error classification move into storage helpers, with scheduler integrations, API tests, documentation, Bazel updates, and test mappings updated accordingly. ChangesDXF error reporting and classification
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant DXFHistoryAPI
participant HistoryStorage
participant ClassifyTaskError
DXFHistoryAPI->>HistoryStorage: list history tasks
HistoryStorage->>ClassifyTaskError: classify stored task state and error
ClassifyTaskError->>HistoryStorage: error category and normalized code
HistoryStorage->>DXFHistoryAPI: structured task summary
Possibly related PRs
Suggested reviewers: Poem
🚥 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 |
| } | ||
|
|
||
| // IsCancelledErr checks if the error is a cancelled error. | ||
| func IsCancelledErr(err error) bool { |
There was a problem hiding this comment.
moved to task_state.go
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkg/dxf/bizutil/error_test.go (1)
24-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse table-driven tests for better coverage.
IsDataErrorimplements multiple conditional string matches for different types of data errors (e.g., constraints, partitions, duplicate keys). It's recommended to use a table-driven test to easily cover all these distinct patterns and ensure none regress over time.♻️ Proposed refactor
-func TestIsDataError(t *testing.T) { - dataErr := errors.New("[Lightning:Restore:ErrEncodeKV]Value conversion failed for column 'a'") - require.True(t, IsDataError(dataErr)) - require.False(t, IsDataError(errors.New("Value conversion failed for column 'a'"))) -} +func TestIsDataError(t *testing.T) { + tests := []struct { + err error + want bool + }{ + {errors.New("[Lightning:Restore:ErrEncodeKV]Value conversion failed for column 'a'"), true}, + {errors.New("[Lightning:Restore:ErrEncodeKV]Check constraint 'c' is violated"), true}, + {errors.New("[Lightning:Restore:ErrEncodeKV]Table has no partition for value 1"), true}, + {errors.New("[executor:8167]Duplicate key conflict found"), true}, + {errors.New("ErrFoundDataConflictRecords found data conflict records"), true}, + {errors.New("ErrFoundIndexConflictRecords found index conflict records"), true}, + {errors.New("[kv:1062]Duplicate entry '1' for key 't.idx'"), true}, + {errors.New("Value conversion failed for column 'a'"), false}, + {errors.New("some random error"), false}, + {nil, false}, + } + for _, tt := range tests { + require.Equal(t, tt.want, IsDataError(tt.err)) + } +}🤖 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 `@pkg/dxf/bizutil/error_test.go` around lines 24 - 28, Refactor TestIsDataError into a table-driven test covering each conditional error pattern handled by IsDataError, including constraints, partitions, duplicate keys, and the existing encode-value case. Include negative cases for unrelated messages, then iterate through the cases and assert the expected result.pkg/dxf/framework/scheduler/BUILD.bazel (1)
70-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRevert cosmetic test-target tweak.
Based on learnings, running
make bazel_prepareis only required when making structural Bazel changes. For cosmetic changes or test-target tweaks that do not affect generated metadata (such as changing theshard_countattribute of ago_testrule in an otherwise structurally unmodified package), do not run or commit themake bazel_prepareoutput. Please consider reverting this line.♻️ Proposed refactor
- shard_count = 45, + shard_count = 46,🤖 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 `@pkg/dxf/framework/scheduler/BUILD.bazel` at line 70, Revert the shard_count change in the affected go_test rule, restoring the prior test-target configuration. Do not include any generated metadata changes from bazel_prepare, since this cosmetic adjustment does not require them.Source: Learnings
pkg/dxf/framework/storage/history.go (1)
237-247: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPrevent potential nil-pointer dereference.
Protect against a nil-pointer dereference in the rare event that
errors.Asmatches a typed nil in the error chain (which causesAsto return true while assigningniltonormalizedErr).🛡️ Proposed defensive check
func taskErrorCode(taskErr error) string { var normalizedErr *errors.Error - if !goerrors.As(taskErr, &normalizedErr) { + if !goerrors.As(taskErr, &normalizedErr) || normalizedErr == nil { return "" } code := string(normalizedErr.RFCCode()) if code == "0" { return "" } return code }🤖 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 `@pkg/dxf/framework/storage/history.go` around lines 237 - 247, Update taskErrorCode to check whether normalizedErr is nil immediately after goerrors.As succeeds, returning an empty string before calling RFCCode. Preserve the existing handling for unmatched errors and RFC code "0".
🤖 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.
Nitpick comments:
In `@pkg/dxf/bizutil/error_test.go`:
- Around line 24-28: Refactor TestIsDataError into a table-driven test covering
each conditional error pattern handled by IsDataError, including constraints,
partitions, duplicate keys, and the existing encode-value case. Include negative
cases for unrelated messages, then iterate through the cases and assert the
expected result.
In `@pkg/dxf/framework/scheduler/BUILD.bazel`:
- Line 70: Revert the shard_count change in the affected go_test rule, restoring
the prior test-target configuration. Do not include any generated metadata
changes from bazel_prepare, since this cosmetic adjustment does not require
them.
In `@pkg/dxf/framework/storage/history.go`:
- Around line 237-247: Update taskErrorCode to check whether normalizedErr is
nil immediately after goerrors.As succeeds, returning an empty string before
calling RFCCode. Preserve the existing handling for unmatched errors and RFC
code "0".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e0c13e4c-a152-4c81-8e95-ef2113511194
📒 Files selected for processing (18)
.agents/skills/tidb-test-guidelines/references/dxf-case-map.mddocs/tidb_http_api.mdpkg/dxf/bizutil/BUILD.bazelpkg/dxf/bizutil/error.gopkg/dxf/bizutil/error_test.gopkg/dxf/framework/scheduler/BUILD.bazelpkg/dxf/framework/scheduler/scheduler.gopkg/dxf/framework/scheduler/scheduler_nokit_test.gopkg/dxf/framework/scheduler/scheduler_test.gopkg/dxf/framework/storage/BUILD.bazelpkg/dxf/framework/storage/history.gopkg/dxf/framework/storage/history_test.gopkg/dxf/framework/storage/table_test.gopkg/dxf/framework/storage/task_state.gopkg/dxf/framework/storage/task_state_test.gopkg/dxf/importinto/scheduler.gopkg/dxf/importinto/scheduler_testkit_test.gopkg/server/handler/tests/dxf_test.go
💤 Files with no reviewable changes (1)
- pkg/dxf/framework/scheduler/scheduler_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69863 +/- ##
================================================
- Coverage 76.3207% 74.0222% -2.2985%
================================================
Files 2041 2057 +16
Lines 559980 580128 +20148
================================================
+ Hits 427381 429424 +2043
- Misses 131698 150213 +18515
+ Partials 901 491 -410
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
# Conflicts: # .agents/skills/tidb-test-guidelines/references/dxf-case-map.md # pkg/server/handler/tests/dxf_test.go
|
🔍 Starting code review for this PR... |
ingress-bot
left a comment
ingress-bot
left a comment
There was a problem hiding this comment.
This review was generated by AI and should be verified by a human reviewer.
Manual follow-up is recommended before merge.
Summary
- Total findings: 8
- Inline comments: 7
- Summary-only findings (no inline anchor): 1
Findings (highest risk first)
🟡 [Minor] (7)
- New bizutil package created for a single caller inside storage (pkg/dxf/bizutil/error.go:21, pkg/dxf/framework/storage/history.go:245)
- ErrorCategory value "failed" is ambiguous between real Failed state and uncategorized Reverted errors (pkg/dxf/framework/storage/history.go:248, docs/tidb_http_api.md:66, pkg/dxf/framework/storage/table_test.go:601)
- getMetricState re-implements ClassifyTaskError's Failed case instead of delegating for all terminal states (pkg/dxf/framework/scheduler/scheduler.go:830, pkg/dxf/framework/storage/history.go:234)
- Classification label strings duplicated across scheduler and storage packages (pkg/dxf/framework/scheduler/scheduler.go:48, pkg/dxf/framework/scheduler/scheduler.go:49, pkg/dxf/framework/storage/history.go:42)
- New taskErrorCategoryCancelled/DataError constants duplicate scheduler.go's metricStateCancelled/DataError under a different name with no shared source (pkg/dxf/framework/storage/history.go:42, pkg/dxf/framework/scheduler/scheduler.go:48, pkg/dxf/framework/scheduler/scheduler.go:837)
- ClassifyTaskError's doc comment calls the result "safe" without saying what it's safe from (pkg/dxf/framework/storage/history.go:233)
- taskErrorCode's "0" sentinel check has no rationale comment (pkg/dxf/framework/storage/history.go:260)
🧹 [Nit] (1)
- Exported TaskCancelMessage/IsCancelledErr lost their design-rationale comment and don't disclose substring matching (pkg/dxf/framework/storage/task_state.go:30, pkg/dxf/framework/storage/task_state.go:34)
Unanchored findings
🟡 [Minor] (1)
- getMetricState re-implements ClassifyTaskError's Failed case instead of delegating for all terminal states
- Request: Have
getMetricStatecallstorage.ClassifyTaskError(state, taskErr)for bothTaskStateFailedandTaskStateReverted(or any stateClassifyTaskErroralready covers) instead of re-implementing the Failed branch locally, and drop the now-unreachableproto.TaskStateFailed.String()fallback after the delegation.
- Request: Have
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
OliverS929
left a comment
OliverS929
left a comment
There was a problem hiding this comment.
LGTM overall.
Non-blocking: please update the PR description to match the current implementation.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: OliverS929, wjhuang2016 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
/retest |
|
/cherry-pick release-nextgen-202603 |
|
@D3Hunter: new pull request created to branch DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
What problem does this PR solve?
Issue Number: ref #67640
Problem Summary:
PR #69739 exposed persisted DXF task error messages through the task history HTTP API so operators could diagnose failed history tasks. However, some task errors can contain sensitive information that is not covered by
tidb_redact_log. Returning the complete error message from an HTTP API can therefore disclose that information.What changed and how does it work?
The task history response no longer exposes the raw task error message.
HistoryTaskSummary.Erroris replaced by:ErrorCode, populated on a best-effort basis from the effective RFC code of a*errors.Error.ErrorCategory, a coarsefailed,cancelled, ordata-errorcategory.The categorization reuses the logic previously used by DXF finished-task metrics. The data-error and cancellation helpers are moved to
dxfutilso the metrics and HTTP history response use the same behavior.The storage and HTTP tests verify named, wrapped, legacy, numeric, code-less, cancelled, data, and ordinary errors. The HTTP regression test also verifies that the sensitive error message and the old
Errorfield are absent from the response.Check List
Tests
./tools/check/failpoint-go-test.sh pkg/dxf/framework/dxfutil -run '^(TestAcquireTaskRuntime|TestCheckTaskRuntime|TestTaskErrorCategory)$' -count=1./tools/check/failpoint-go-test.sh pkg/dxf/framework/storage -run '^TestTaskHistoryTable$' -count=1./tools/check/failpoint-go-test.sh pkg/dxf/framework/scheduler -run '^(TestBaseSchedulerInitChecksTaskRuntime|TestOnTaskFinished)$' -count=1./tools/check/failpoint-go-test.sh pkg/dxf/framework/taskexecutor -run '^TestBaseTaskExecutorInitChecksTaskRuntime$' -count=1./tools/check/failpoint-go-test.sh pkg/dxf/importinto -run '^TestSchedulerExtLocalSort$' -count=1./tools/check/failpoint-go-test.sh pkg/dxf/importinto -tags=intest,deadlock,nextgen -run '^(TestSchedulerExtLocalSort|TestSchedulerPrepareEnabledJobTransitionsFromPreparingToFirstBusinessPhase)$' -count=1./tools/check/failpoint-go-test.sh pkg/server/handler/tests -run '^TestDXFAPI$/^task_history_api$' -count=1./tools/check/failpoint-go-test.sh pkg/server/handler/tests -tags=intest,deadlock,nextgen -run '^TestDXFAPI$/^task_history_api$' -count=1make lintSide effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Documentation
Tests