Repository navigation
[fix][evaluation] fix export URL encoding, add upload retry, trajectory lower-bound, goroutine leak & long-conn target validation - #569
Merged
Conversation
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #569 +/- ##
==========================================
+ Coverage 78.12% 78.13% +0.01%
==========================================
Files 670 670
Lines 76365 76404 +39
==========================================
+ Hits 59658 59701 +43
+ Misses 13266 13264 -2
+ Partials 3441 3439 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
xueyizheng
force-pushed
the
fix/export-url-path-encoding
branch
2 times, most recently
from
July 6, 2026 07:13
6101c65 to
6361bcc
Compare
When generating the experiment-result CSV export download link, ProcessSignURL unescaped the entire signed URL (url.QueryUnescape over the whole string) to make Chinese filenames human-readable. This also decodes the percent-encoding of structural characters such as '[' ']' '/' and space in the path, which breaks downloads: 1. The signature (e.g. x-signature) is computed over the ENCODED path; once the path is rewritten the signature no longer matches. 2. A '/' in the filename is turned back into a real path separator, corrupting the object key structure -> 404. This surfaces as "export download always fails" when the experiment name contains '[' ']' '/' (e.g. "[auto]xxx"). Chinese-only names happened to keep working because the decoded path stayed a valid path. Fix: keep the path/query percent-encoding byte-for-byte as emitted by the signing service (preserve EscapedPath + RawQuery); only strip scheme+host for the localos case. Correct for Chinese, '[' ']', '/' and spaces alike. Readable filenames are delegated to the download response headers (Content-Disposition). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ient errors Large record fields (evaluator/eval-target input & output Content) are offloaded to object storage (ImageX/TOS) in record_data.go. The ApplyImageUpload path occasionally returns transient errors (e.g. 201007 / bad gateway) caused by single-instance migration on the storage side; the platform auto-recovers and a retry succeeds. This surfaced as sporadic export/record failures where, within one turn, one field uploads fine while the next fails. Fix (per ImageX oncall's first recommendation): wrap the upload with a fixed-interval retry — up to 2 retries (3 attempts total), 1.5s interval — and rebuild the reader on every attempt so a consumed reader from a failed attempt does not upload empty content. Adds a reusable backoff.RetryWithMaxTimesAndInterval helper (constant interval, bounded retries). Tests: existing upload-error cases updated to expect 3 attempts; added Test_processContent_upload_retry_success covering fail-fail-succeed with reader rebuild verification. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…bound for async targets For async eval targets, ReportInvokeRecords extracted the trajectory using record.BaseInfo.CreatedAt as the trace time lower bound. That timestamp is stamped only after the async invocation returns (in asyncExecuteTarget, time.Now() is taken after operator.AsyncExecute), so for slow targets (e.g. web agent: sandbox setup, file writes, command exec) it can be seconds later than when the request was actually issued. Since the trace query window is [start, now] with no buffer, spans emitted between request start and return get cut off -> trajectory is incomplete or empty. Fix: thread the request-start time (EvalAsyncCtx.AsyncUnixMS, captured right before submitting the async call and already used for TimeConsumingMS) into ReportTargetRecordParam, and use it as the extraction lower bound. Fall back to record.BaseInfo.CreatedAt when it is 0, keeping backward compatibility. Tests: added TestEvalTargetServiceImpl_ReportInvokeRecords_TrajectoryStartTime covering AsyncUnixMS-preferred and CreatedAt-fallback. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Defensive margin on top of the request-start-time fix: ExtractTrajectory now subtracts a 1-minute buffer from the trace query lower bound to absorb possible clock/report-latency skew between the request-start timestamp and the actual span emit time, avoiding dropping the earliest spans. The trace query matches by traceID, so widening the lower bound does not pull in unrelated data. nil lower bound stays nil. Applies to all trajectory extraction paths (sync and async) since they all go through ExtractTrajectory. Tests: added TestEvalTargetServiceImpl_ExtractTrajectory_StartTimeBuffer; updated the async start-time test expectations to include the buffer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…return callEvaluators builds a goroutine.Pool (goroutine.NewPool) and only releases the underlying ants pool inside exec() via `defer p.p.Release()`. When the loop between NewPool and pool.ExecAll returns early (evaluator conf is nil, or buildEvaluatorInputData fails), ExecAll is skipped, so exec() never runs and Release() never fires. The ants pool's two resident goroutines (purge + ticktock) can only be stopped by Release, and IPool did not expose Release, so the pool leaks. callEvaluators is a per-turn hot path and these error branches are not rare (e.g. custom_rpc downstream failures), so the leak accumulates (~2 goroutines per miss; observed ~3900 zombie goroutines / 80% of total after ~14h in prod). Fix: bind release to the pool lifecycle rather than to execution. - Expose Release() on IPool; make it idempotent via sync.Once. - exec() now defers p.Release() (idempotent) instead of p.p.Release(). - callEvaluators defers pool.Release() right after NewPool succeeds, so any early return still releases the pool. The other 4 goroutine.NewPool sites in the module already reach Exec unconditionally, so they were never leaking; the extra idempotent Release is harmless to them. Tests: added TestPool_Release_WithoutExec covering release-without-exec, idempotency, release-after-ExecAll, and a no-goroutine-leak assertion (50x NewPool+Release stays flat). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…turn path Adds a regression test at the actual leak site (callEvaluators via the public CallEvaluators), not just the low-level pool. It drives the "evaluator conf not found" early return (which skips pool.ExecAll) 50 times and asserts runtime.NumGoroutine does not grow. Verified the test catches the bug: with the fix disabled (defer pool.Release() removed) it fails with delta=100 (2 leaked resident goroutines × 50 calls); with the fix it passes (delta≈0). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
新增的 RetryWithMaxTimesAndInterval 之前无直接单测(仅被 record_data 间接调用), 补充覆盖:重试后成功 / 达上限失败 / 首次成功不重试 / 固定间隔生效。 backoff 包覆盖率 72.2% -> 83.3%。 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The ImageX/TOS large-field upload now retries up to 3 attempts on error (record_data.go). Repo-layer tests that mock Upload to always fail expected exactly 1 call; adjust to .Times(3) so the retry does not trip 'unexpected call' in eval_target / evaluator_record repo tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ts in OpenAPI submit OpenAPI SubmitExperimentOApi accepts long-connection eval targets (custom_agent / a2a_agent / custom_rpc_server) but never validated the required cluster/env. A missing value passed creation silently and only failed at run time with an opaque RPC error (spi GetClient with empty cluster), so users saw "experiment run failed" instead of a clear param error at submit time. Add ValidateOpenAPIEvalTargetClusterEnv (convertor) and call it in the submit handler right after the target-type whitelist check: - custom_agent / a2a_agent: cluster + env required (both caller-supplied) - custom_rpc_server: only env required (cluster comes from app config) Error messages include common values (cluster e.g. "default", env e.g. "cn"). Uses errno.CommonInvalidParamCode, consistent with other submit param checks. No IDL / commercial change needed. Tests: TestValidateOpenAPIEvalTargetClusterEnv covers all three types (missing cluster / missing env / both present), non-long-connection skip, and nil. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…gion enum
env (exec_env) is a lane / X-TT-ENV identifier (free-form string, e.g.
"ppe_fornax_eval"), not a boe/ppe/online/cn enum. The validation stays
'non-empty required'; only the error-message example is corrected from
"cn" to "ppe_fornax_eval" to avoid misleading users. cluster hint
("default") is unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… (direct-connect) custom_agent / a2a_agent with an explicit AgentConnection are dispatched by the frontier tuple (ProductID/AppID/UserID/DeviceID); cluster/env are not used at run time. Requiring them would wrongly reject the legitimate direct-connect path (even though no such data exists in prod today, the code path is supported and must not be broken). Skip cluster/env validation when AgentConnection is set. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Per current scope, only custom_agent (the in-house long-connection agent) requires cluster/env at submit (with AgentConnection direct-connect exempted). a2a_agent and custom_rpc_server are intentionally left un-validated for now. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- ValidateOpenAPIEvalTargetClusterEnv doc comment matched the pre-narrowing behavior (claimed to validate a2a/custom_rpc); correct it to reflect that only custom_agent is validated now. - record_data.go: note that the upload retry currently retries ALL errors indiscriminately (no transient/permanent distinction); suggest backoff.Permanent for non-transient errors if an error-code set is known later. Comment-only, no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
xueyizheng
force-pushed
the
fix/export-url-path-encoding
branch
from
July 6, 2026 09:14
6361bcc to
b3d6c37
Compare
lizwang11
approved these changes
Jul 6, 2026
Tiny1028
approved these changes
Jul 6, 2026
Colin4k1024
pushed a commit
to Colin4k1024/coze-loop
that referenced
this pull request
Aug 28, 2026
…ry lower-bound, goroutine leak & long-conn target validation (coze-dev#569) * [fix][evaluation] preserve signed URL path encoding in ProcessSignURL When generating the experiment-result CSV export download link, ProcessSignURL unescaped the entire signed URL (url.QueryUnescape over the whole string) to make Chinese filenames human-readable. This also decodes the percent-encoding of structural characters such as '[' ']' '/' and space in the path, which breaks downloads: 1. The signature (e.g. x-signature) is computed over the ENCODED path; once the path is rewritten the signature no longer matches. 2. A '/' in the filename is turned back into a real path separator, corrupting the object key structure -> 404. This surfaces as "export download always fails" when the experiment name contains '[' ']' '/' (e.g. "[auto]xxx"). Chinese-only names happened to keep working because the decoded path stayed a valid path. Fix: keep the path/query percent-encoding byte-for-byte as emitted by the signing service (preserve EscapedPath + RawQuery); only strip scheme+host for the localos case. Correct for Chinese, '[' ']', '/' and spaces alike. Readable filenames are delegated to the download response headers (Content-Disposition). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * [fix][evaluation] retry large-field upload to object storage on transient errors Large record fields (evaluator/eval-target input & output Content) are offloaded to object storage (ImageX/TOS) in record_data.go. The ApplyImageUpload path occasionally returns transient errors (e.g. 201007 / bad gateway) caused by single-instance migration on the storage side; the platform auto-recovers and a retry succeeds. This surfaced as sporadic export/record failures where, within one turn, one field uploads fine while the next fails. Fix (per ImageX oncall's first recommendation): wrap the upload with a fixed-interval retry — up to 2 retries (3 attempts total), 1.5s interval — and rebuild the reader on every attempt so a consumed reader from a failed attempt does not upload empty content. Adds a reusable backoff.RetryWithMaxTimesAndInterval helper (constant interval, bounded retries). Tests: existing upload-error cases updated to expect 3 attempts; added Test_processContent_upload_retry_success covering fail-fail-succeed with reader rebuild verification. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * [fix][evaluation] use request-start time as trajectory extract lower bound for async targets For async eval targets, ReportInvokeRecords extracted the trajectory using record.BaseInfo.CreatedAt as the trace time lower bound. That timestamp is stamped only after the async invocation returns (in asyncExecuteTarget, time.Now() is taken after operator.AsyncExecute), so for slow targets (e.g. web agent: sandbox setup, file writes, command exec) it can be seconds later than when the request was actually issued. Since the trace query window is [start, now] with no buffer, spans emitted between request start and return get cut off -> trajectory is incomplete or empty. Fix: thread the request-start time (EvalAsyncCtx.AsyncUnixMS, captured right before submitting the async call and already used for TimeConsumingMS) into ReportTargetRecordParam, and use it as the extraction lower bound. Fall back to record.BaseInfo.CreatedAt when it is 0, keeping backward compatibility. Tests: added TestEvalTargetServiceImpl_ReportInvokeRecords_TrajectoryStartTime covering AsyncUnixMS-preferred and CreatedAt-fallback. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * [fix][evaluation] add 1-minute buffer to trajectory extract lower bound Defensive margin on top of the request-start-time fix: ExtractTrajectory now subtracts a 1-minute buffer from the trace query lower bound to absorb possible clock/report-latency skew between the request-start timestamp and the actual span emit time, avoiding dropping the earliest spans. The trace query matches by traceID, so widening the lower bound does not pull in unrelated data. nil lower bound stays nil. Applies to all trajectory extraction paths (sync and async) since they all go through ExtractTrajectory. Tests: added TestEvalTargetServiceImpl_ExtractTrajectory_StartTimeBuffer; updated the async start-time test expectations to include the buffer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * [fix][evaluation] fix goroutine pool leak in callEvaluators on early return callEvaluators builds a goroutine.Pool (goroutine.NewPool) and only releases the underlying ants pool inside exec() via `defer p.p.Release()`. When the loop between NewPool and pool.ExecAll returns early (evaluator conf is nil, or buildEvaluatorInputData fails), ExecAll is skipped, so exec() never runs and Release() never fires. The ants pool's two resident goroutines (purge + ticktock) can only be stopped by Release, and IPool did not expose Release, so the pool leaks. callEvaluators is a per-turn hot path and these error branches are not rare (e.g. custom_rpc downstream failures), so the leak accumulates (~2 goroutines per miss; observed ~3900 zombie goroutines / 80% of total after ~14h in prod). Fix: bind release to the pool lifecycle rather than to execution. - Expose Release() on IPool; make it idempotent via sync.Once. - exec() now defers p.Release() (idempotent) instead of p.p.Release(). - callEvaluators defers pool.Release() right after NewPool succeeds, so any early return still releases the pool. The other 4 goroutine.NewPool sites in the module already reach Exec unconditionally, so they were never leaking; the extra idempotent Release is harmless to them. Tests: added TestPool_Release_WithoutExec covering release-without-exec, idempotency, release-after-ExecAll, and a no-goroutine-leak assertion (50x NewPool+Release stays flat). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * [test][evaluation] add goroutine-leak test at callEvaluators early-return path Adds a regression test at the actual leak site (callEvaluators via the public CallEvaluators), not just the low-level pool. It drives the "evaluator conf not found" early return (which skips pool.ExecAll) 50 times and asserts runtime.NumGoroutine does not grow. Verified the test catches the bug: with the fix disabled (defer pool.Release() removed) it fails with delta=100 (2 leaked resident goroutines × 50 calls); with the fix it passes (delta≈0). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * [test][evaluation] gofmt fix comment alignment in new tests Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * [test][infra] cover RetryWithMaxTimesAndInterval (diff coverage) 新增的 RetryWithMaxTimesAndInterval 之前无直接单测(仅被 record_data 间接调用), 补充覆盖:重试后成功 / 达上限失败 / 首次成功不重试 / 固定间隔生效。 backoff 包覆盖率 72.2% -> 83.3%。 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * [test][evaluation] update Upload-error mocks for TOS upload retry The ImageX/TOS large-field upload now retries up to 3 attempts on error (record_data.go). Repo-layer tests that mock Upload to always fail expected exactly 1 call; adjust to .Times(3) so the retry does not trip 'unexpected call' in eval_target / evaluator_record repo tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * [fix][evaluation] validate cluster/env for long-connection eval targets in OpenAPI submit OpenAPI SubmitExperimentOApi accepts long-connection eval targets (custom_agent / a2a_agent / custom_rpc_server) but never validated the required cluster/env. A missing value passed creation silently and only failed at run time with an opaque RPC error (spi GetClient with empty cluster), so users saw "experiment run failed" instead of a clear param error at submit time. Add ValidateOpenAPIEvalTargetClusterEnv (convertor) and call it in the submit handler right after the target-type whitelist check: - custom_agent / a2a_agent: cluster + env required (both caller-supplied) - custom_rpc_server: only env required (cluster comes from app config) Error messages include common values (cluster e.g. "default", env e.g. "cn"). Uses errno.CommonInvalidParamCode, consistent with other submit param checks. No IDL / commercial change needed. Tests: TestValidateOpenAPIEvalTargetClusterEnv covers all three types (missing cluster / missing env / both present), non-long-connection skip, and nil. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * [fix][evaluation] correct env hint: it is a lane identifier, not a region enum env (exec_env) is a lane / X-TT-ENV identifier (free-form string, e.g. "ppe_fornax_eval"), not a boe/ppe/online/cn enum. The validation stays 'non-empty required'; only the error-message example is corrected from "cn" to "ppe_fornax_eval" to avoid misleading users. cluster hint ("default") is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * [fix][evaluation] exempt cluster/env when AgentConnection is provided (direct-connect) custom_agent / a2a_agent with an explicit AgentConnection are dispatched by the frontier tuple (ProductID/AppID/UserID/DeviceID); cluster/env are not used at run time. Requiring them would wrongly reject the legitimate direct-connect path (even though no such data exists in prod today, the code path is supported and must not be broken). Skip cluster/env validation when AgentConnection is set. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * [fix][evaluation] narrow long-conn validation to custom_agent only Per current scope, only custom_agent (the in-house long-connection agent) requires cluster/env at submit (with AgentConnection direct-connect exempted). a2a_agent and custom_rpc_server are intentionally left un-validated for now. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * [docs][evaluation] fix stale doc comment & clarify retry semantics - ValidateOpenAPIEvalTargetClusterEnv doc comment matched the pre-narrowing behavior (claimed to validate a2a/custom_rpc); correct it to reflect that only custom_agent is validated now. - record_data.go: note that the upload retry currently retries ALL errors indiscriminately (no transient/permanent distinction); suggest backoff.Permanent for non-transient errors if an error-code set is known later. Comment-only, no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Preserve signed-URL path encoding in ProcessSignURLThe experiment CSV export download link had its whole URL unescaped, which decoded the percent-encoding of the path (e.g. [, ], /, spaces, non-ASCII). That broke the signature check and corrupted the object key, so exports whose experiment name contained such characters failed to download. The path/query encoding is now passed through byte-for-byte as issued by the signing service; readable filenames are left to the download response headers.
Retry large-field upload to object storage on transient errorsLarge record fields are offloaded to object storage; the upload could fail on transient, self-healing errors. Added a fixed-interval, bounded retry helper (backoff.RetryWithMaxTimesAndInterval) and wrapped the upload with it (2 retries, 1.5s apart), rebuilding the reader on each attempt so a consumed reader never re-uploads empty content.
Use request-start time as the trajectory extraction lower bound for async targetsAsync eval targets used the record's created-at time (stamped after the async call returns) as the trace query lower bound, which dropped early spans. It now uses the request-start time, minus a 1-minute buffer, and falls back to created-at when the start time is absent.
Fix goroutine pool leak in callEvaluators on early returnWhen callEvaluators returned early (missing evaluator conf / input build failure) it skipped pool.ExecAll, so the underlying pool's resident goroutines were never released. IPool.Release() is now exposed and idempotent (sync.Once), and is deferred right after pool creation so any early return still releases it.