expression, types: freeze unsafe datetime warning args#66870
expression, types: freeze unsafe datetime warning args#66870D3Hunter wants to merge 4 commits intopingcap:masterfrom
Conversation
ti-chi-bot
bot
added
the
release-note
|
❌ Failed to Create Project We encountered an error while creating your project. Please try again or contact support if the issue persists. |
ti-chi-bot
bot
added
the
size/L
|
Hi @D3Hunter. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions 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 kubernetes-sigs/prow repository. |
|
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 ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis change introduces a lightweight string abstraction (String, PlainStr, HackedStr with FreezeStr) and switches time parsing APIs to accept the abstraction to freeze warning arguments; it adds a unit test for warning stability and updates two Go dependencies ( Changes
Sequence Diagram(s)sequenceDiagram
participant Chunk as Chunk (mutable)
participant Expr as Expr (strDatetimeAdd/Sub)
participant HStr as HackedStr
participant Types as ParseTimeWithString
participant ErrCtx as Warning (errors)
Chunk->>Expr: GetString(i) (zero-copy)
Expr->>HStr: wrap input as HackedStr
Expr->>Types: ParseTimeWithString(ctx, HStr, ...)
Types->>HStr: call FreezeStr()
HStr->>Types: return frozen cloned string
Types->>ErrCtx: AppendWarning(frozen string)
Note over ErrCtx: Warning args now independent of chunk memory
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Command failed 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 |
|
/ok-to-test |
|
/retest |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/expression/builtin_time.go`:
- Line 4824: The duration parsing paths still pass the raw string into
types.ParseDuration, which can produce ErrTruncatedWrongVal that embeds the
input and reintroduces the warning mutation vulnerability; update the two
helpers strDurationAddDuration and strDurationSubDuration to wrap the d
parameter with types.HackedStr (i.e. call types.ParseDuration(tc,
types.HackedStr(d), ...)) before parsing, mirroring the datetime fix you applied
for ParseTimeWithString, and add regression tests covering duration-shaped
inputs to ensure warnings no longer expose the raw string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 778dbe31-10dd-469d-a4b4-540d3440df2e
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
DEPS.bzlgo.modpkg/expression/builtin_time.gopkg/expression/builtin_time_test.gopkg/types/BUILD.bazelpkg/types/string.gopkg/types/time.go
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: windtalker The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
Codecov Report❌ Patch coverage is Please upload reports for the commit 76b33e6 to get more accurate results. Additional details and impacted files@@ Coverage Diff @@
## master #66870 +/- ##
================================================
- Coverage 77.6758% 77.1740% -0.5019%
================================================
Files 2009 1930 -79
Lines 549988 539955 -10033
================================================
- Hits 427208 416705 -10503
- Misses 121071 122952 +1881
+ Partials 1709 298 -1411
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@D3Hunter: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| require.Eventually(t, | ||
| // Wait for the GC triggered by memory810mb | ||
| func() bool { | ||
| runtime.GC() |
There was a problem hiding this comment.
Adding runtime.GC() here makes this polling deterministic. After dependency updates (especially testify timing behavior), this test no longer always gets enough GC opportunities within the original wait window, causing flakes/timeouts even when tuner logic is correct.
|
@D3Hunter: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
What problem does this PR solve?
Issue Number: close #66834, close #56311, close #65231
Problem Summary:
SUBTIME/ADDTIMEwarning texts can become nondeterministic when the datetime input string comes from chunk-backed zero-copy memory. The warning message is formatted later, after chunk buffers may be reused, so the error argument can be corrupted and flaky tests appear as random warning values.in pingcap/errors#82, we support freeze string args
What changed and how does it work?
pkg/types:types.PlainStrfor normal stable strings.types.HackedStrfor potentially aliased/unsafe strings; it implementsFreezeStr()sopingcap/errorscan snapshot arguments.types.ParseTimeWithStringand route internal datetime parse paths to accepttypes.Stringwrappers.parseDatetime, timestamp DST adjustment) to pass wrapped string args instead of raw strings.strDatetimeAddDurationandstrDatetimeSubDurationto callParseTimeWithString(..., types.HackedStr(d), ...), so warnings generated from chunk-backed inputs are frozen.TestStrDatetimeAddDurationFreezesWarningArgto verify warning args do not mutate after backing buffer rewrite.github.com/pingcap/errorstov0.11.5-0.20260310054046-9c8b3586e4b2(includes hacked-string freezing support) and sync Bazel metadata.Check List
Tests
Manual verification steps:
Real cluster result:
mismatch_count=0Side effects
Documentation
Release note
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores