[codex] Support TiDB Cloud Native e2e provisioning#677
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (31)
📝 WalkthroughWalkthroughAdds shared e2e helpers for TiDB Cloud Native provisioning, registry-based cleanup, and temporary-directory handling. Smoke runners and test scripts now use the shared flows, while documentation covers hosted credentials, cleanup retries, temp roots, and scheduled failure handling. ChangesHosted e2e infrastructure and smoke coverage
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
fa605b2 to
62d9275
Compare
2df9690 to
a83676e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
e2e/fuse-posix-fsx-gate.sh (1)
614-614: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winProvisioning call loses 403/5xx retry coverage.
This file's own
curl_body_code(lines 209-240) retried provisioning on429,403, and any5xxresponse. The newdrive9_provision_curl_body_codepath only retries429(perdrive9_provision_to_file'sif [ "$code" != "429" ] ...check). This is the richest retry logic lost among the scripts in this batch — a transient403or5xxduring the fsx-gate run will now fail immediately instead of retrying.Same root-cause concern as flagged in
e2e/fuse-concurrency-stress.sh(Line 702); the fix belongs inprovision-helper.sh's retry condition.🤖 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 `@e2e/fuse-posix-fsx-gate.sh` at line 614, The provisioning path now only retries on 429, dropping the existing 403 and 5xx retry coverage used by the fsx-gate flow. Update the retry condition in provision-helper.sh inside drive9_provision_to_file so it matches the broader behavior previously handled by curl_body_code, and ensure drive9_provision_curl_body_code can still retry transient 403 and 5xx responses instead of failing immediately.
🧹 Nitpick comments (2)
e2e/smoke-all.sh (1)
43-49: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard
drive9_cleanup_init_runfailure.The return value is ignored. If setup fails (e.g.,
mkdir/chmod/validation), the registry env vars aren't exported, buttrap cleanup_exit EXITis still installed and every sub-suite'sdrive9_cleanup_require_readywill then fail, turning a setup error into cascading suite failures. Abort early instead.♻️ Suggested guard
if [ "$CLEANUP_ENABLED" = "1" ]; then if [ "${DRIVE9_E2E_CLEANUP_PENDING:-1}" = "1" ]; then drive9_cleanup_run_pending fi - drive9_cleanup_init_run + drive9_cleanup_init_run || { echo "cleanup init failed; aborting" >&2; exit 1; } trap cleanup_exit EXIT fi🤖 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 `@e2e/smoke-all.sh` around lines 43 - 49, `drive9_cleanup_init_run` is currently called without checking whether setup succeeded, so failures still leave `trap cleanup_exit EXIT` installed and cause later `drive9_cleanup_require_ready` errors. In `e2e/smoke-all.sh`, update the `CLEANUP_ENABLED` block to check the return from `drive9_cleanup_init_run` and abort immediately if it fails, only installing `trap cleanup_exit EXIT` after a successful init; use the existing `drive9_cleanup_init_run`, `cleanup_exit`, and `drive9_cleanup_require_ready` flow to locate the fix.e2e/cleanup-helper.sh (1)
415-425: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winVerification runs immediately after an async 202 delete with no poll/backoff.
DELETEreturns202(accepted, async), butdrive9_cleanup_verify_deletedqueries/v1/statusimmediately. A tenant still mid-deletion may return200/active, which is counted as a failure, keeps the registry pending, and makesdrive9_cleanup_run_registryreturn non-zero for otherwise-successful deletions. Consider a short bounded poll (a few retries with sleep) for thedeleting|deleted/401|403|404terminal states before declaring failure.🤖 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 `@e2e/cleanup-helper.sh` around lines 415 - 425, The post-DELETE verification in drive9_cleanup_verify_deleted is running too early after an async 202 response, so cleanup can fail while the tenant is still transitioning. Update the verification flow in cleanup-helper.sh to add a short bounded poll/backoff loop before treating the delete as failed, and only return failure after repeated checks still don’t reach a terminal state. Use the existing drive9_cleanup_verify_deleted and drive9_cleanup_run_registry paths as the place to wait for deleting|deleted or 401|403|404 before marking the registry entry pending.
🤖 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.
Inline comments:
In `@e2e/cleanup-helper.sh`:
- Around line 302-316: Persist the TiDB Cloud cleanup credentials needed for
deferred retries: drive9_cleanup_body_args currently rebuilds the DELETE body
only from environment variables, but cleanup-pending.sh later replays pending
tidb_cloud_native entries without its own TiDB Cloud keys. Update the
registry/pending cleanup flow so the native DELETE request can be reconstructed
from stored data, or make the pending replay logic explicitly require and carry
the TiDB Cloud public/private keys before invoking drive9_cleanup_body_args and
related replay paths.
In `@e2e/fuse-smoke-test.sh`:
- Line 818: The provisioning response assignment in the smoke test script should
be guarded so a non-zero return from drive9_provision_curl_body_code does not
terminate the shell early under set -euo pipefail. Update the resp capture near
the POST /v1/provision check to use a fallback assignment (for example, preserve
a default empty value on failure) so the existing check_eq validation and the
cleanup/summary path still run.
---
Duplicate comments:
In `@e2e/fuse-posix-fsx-gate.sh`:
- Line 614: The provisioning path now only retries on 429, dropping the existing
403 and 5xx retry coverage used by the fsx-gate flow. Update the retry condition
in provision-helper.sh inside drive9_provision_to_file so it matches the broader
behavior previously handled by curl_body_code, and ensure
drive9_provision_curl_body_code can still retry transient 403 and 5xx responses
instead of failing immediately.
---
Nitpick comments:
In `@e2e/cleanup-helper.sh`:
- Around line 415-425: The post-DELETE verification in
drive9_cleanup_verify_deleted is running too early after an async 202 response,
so cleanup can fail while the tenant is still transitioning. Update the
verification flow in cleanup-helper.sh to add a short bounded poll/backoff loop
before treating the delete as failed, and only return failure after repeated
checks still don’t reach a terminal state. Use the existing
drive9_cleanup_verify_deleted and drive9_cleanup_run_registry paths as the place
to wait for deleting|deleted or 401|403|404 before marking the registry entry
pending.
In `@e2e/smoke-all.sh`:
- Around line 43-49: `drive9_cleanup_init_run` is currently called without
checking whether setup succeeded, so failures still leave `trap cleanup_exit
EXIT` installed and cause later `drive9_cleanup_require_ready` errors. In
`e2e/smoke-all.sh`, update the `CLEANUP_ENABLED` block to check the return from
`drive9_cleanup_init_run` and abort immediately if it fails, only installing
`trap cleanup_exit EXIT` after a successful init; use the existing
`drive9_cleanup_init_run`, `cleanup_exit`, and `drive9_cleanup_require_ready`
flow to locate the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d8a30e0e-3296-4515-89a0-2895cdea7784
📒 Files selected for processing (23)
e2e/AGENTS.mde2e/README.mde2e/_feature-matrix-runner.she2e/api-smoke-test.she2e/cleanup-helper.she2e/cleanup-pending.she2e/cli-smoke-test.she2e/fuse-concurrency-stress.she2e/fuse-correctness-workload.she2e/fuse-crash-recovery-test.she2e/fuse-performance-baseline.she2e/fuse-posix-fsx-gate.she2e/fuse-smoke-test.she2e/fuse-sqlite-correctness.she2e/fuse-write-perf-budget-test.she2e/git-ops-smoke-test.she2e/git-workspace-smoke-test.she2e/journal-smoke-test.she2e/layer-fs-smoke-test.she2e/portable-pack-unpack-e2e.she2e/posix-permission-smoke-test.she2e/provision-helper.she2e/smoke-all.sh
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0dcb2dec64
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| code=$(curl -sS -o "$output_file" -w "%{http_code}" -X POST "$base/v1/provision") | ||
| fi | ||
|
|
||
| if [ "$code" != "429" ] || [ "$attempt" -ge "$max_retries" ]; then |
There was a problem hiding this comment.
When this helper replaces each suite's local /v1/provision call, this condition now retries only HTTP 429; I checked e2e/layer-fs-smoke-test.sh, whose existing wrapper retried 000|429|5??, and e2e/fuse-correctness-workload.sh, which also retried transient 403s. On hosted runs where provisioning briefly returns a connection failure, 5xx, or 403 throttling, those suites now fail immediately instead of honoring their configured retry budget, making scheduled e2e runs much flakier. Please carry forward the prior retry classes and guard non-zero curl exits before returning.
Useful? React with 👍 / 👎.
| drive9_cleanup_validate_registry_name "$registry" || return 1 | ||
| drive9_cleanup_validate_registry_run_id "$registry" "$run_id" || return 1 | ||
|
|
||
| umask 077 |
There was a problem hiding this comment.
When DRIVE9_E2E_CLEANUP=always or success is used with smoke-all.sh, drive9_cleanup_init_run runs before any child suite and this permanent umask 077 is inherited by all later e2e scripts. That changes default modes for files and directories created through local fixtures, Git, and FUSE from the caller's normal environment to 0600/0700, so cleanup-enabled scheduled runs no longer exercise the same permission behavior as ordinary runs and can mask or create mode-sensitive failures. Capture the previous umask and restore it after creating/chmodding the registry files.
Useful? React with 👍 / 👎.
| if [ -n "$body" ]; then | ||
| code=$(curl -sS -o "$output_file" -w "%{http_code}" -X POST \ | ||
| -H "Content-Type: application/json" \ | ||
| --data-binary "$body" \ |
There was a problem hiding this comment.
On credentialed TiDB Cloud Native e2e runs, $body contains private_key, and passing it directly as the curl --data-binary argument exposes the key in the curl process argv while the request is in flight. This is avoidable by writing the JSON to a 0600 temp file or streaming it on stdin, so the scheduled smoke host does not leak TiDB Cloud credentials via process listings.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,12 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
This adds a new executable under e2e/, but I checked .github/workflows/local-e2e.yml and the README's CI automation tiers section and cleanup-pending.sh is neither wired into automation nor documented there as manual-only with a reason. That violates e2e/AGENTS.md's CI wiring rule for new e2e scripts, so future maintainers cannot tell whether this entrypoint is intentionally manual or simply dead code.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/posix-permission-smoke-test.sh (1)
291-291: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard the provisioning response assignment.
e2e/posix-permission-smoke-test.sh:291runs underset -euo pipefail, soresp=$(drive9_provision_curl_body_code "$BASE")will abort the script on a helper failure and skip the remaining checks and cleanup. Use a guarded assignment here, e.g.resp=$(...) || resp="".🤖 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 `@e2e/posix-permission-smoke-test.sh` at line 291, The provisioning response assignment in the posix permission smoke test is not guarded, so a helper failure can terminate the script early under set -euo pipefail. Update the resp assignment in the smoke test flow to tolerate drive9_provision_curl_body_code failures by using a guarded assignment and preserving the remaining checks and cleanup. Keep the fix localized around the resp capture in the provisioning step so the rest of the script can continue handling the failure path.
🧹 Nitpick comments (1)
e2e/tmp-helper.sh (1)
4-4: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused load-guard variable.
DRIVE9_TMP_HELPER_LOADEDis set but never checked, so it doesn't actually prevent duplicate sourcing (flagged by shellcheck SC2034). Either implement the guard or drop the variable.♻️ Optional: implement an actual idempotent-load guard
-DRIVE9_TMP_HELPER_LOADED=1 +if [ -n "${DRIVE9_TMP_HELPER_LOADED:-}" ]; then + return 0 2>/dev/null || exit 0 +fi +DRIVE9_TMP_HELPER_LOADED=1🤖 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 `@e2e/tmp-helper.sh` at line 4, The load-guard variable in the tmp helper is set but never used, so the script is not actually idempotent. Update the tmp helper sourcing logic to either check DRIVE9_TMP_HELPER_LOADED before loading and exit early on repeat loads, or remove the variable entirely if you do not want a guard; reference DRIVE9_TMP_HELPER_LOADED and the tmp-helper script’s top-level initialization block when making the change.Source: Linters/SAST tools
🤖 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.
Outside diff comments:
In `@e2e/posix-permission-smoke-test.sh`:
- Line 291: The provisioning response assignment in the posix permission smoke
test is not guarded, so a helper failure can terminate the script early under
set -euo pipefail. Update the resp assignment in the smoke test flow to tolerate
drive9_provision_curl_body_code failures by using a guarded assignment and
preserving the remaining checks and cleanup. Keep the fix localized around the
resp capture in the provisioning step so the rest of the script can continue
handling the failure path.
---
Nitpick comments:
In `@e2e/tmp-helper.sh`:
- Line 4: The load-guard variable in the tmp helper is set but never used, so
the script is not actually idempotent. Update the tmp helper sourcing logic to
either check DRIVE9_TMP_HELPER_LOADED before loading and exit early on repeat
loads, or remove the variable entirely if you do not want a guard; reference
DRIVE9_TMP_HELPER_LOADED and the tmp-helper script’s top-level initialization
block when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9549f6fc-6073-4b64-b64b-3b7044f403b2
📒 Files selected for processing (29)
e2e/AGENTS.mde2e/README.mde2e/_feature-matrix-runner.she2e/api-smoke-test.she2e/cleanup-helper.she2e/cleanup-pending.she2e/cli-smoke-test.she2e/fuse-concurrency-stress.she2e/fuse-correctness-workload.she2e/fuse-crash-recovery-test.she2e/fuse-performance-baseline.she2e/fuse-posix-fsx-gate.she2e/fuse-smoke-test.she2e/fuse-sqlite-correctness.she2e/fuse-write-perf-budget-test.she2e/git-ops-smoke-test.she2e/git-workspace-smoke-test.she2e/layer-fs-smoke-test.she2e/local-smoke.she2e/native-smoke-test.she2e/portable-pack-unpack-e2e.she2e/posix-permission-smoke-test.she2e/provision-helper.she2e/server-quota/quota-server-e2e.she2e/smoke-all.she2e/tmp-helper.she2e/verify-description-e2e.she2e/verify-description-tidb-zero-e2e.shscripts/verify_description.sh
✅ Files skipped from review due to trivial changes (1)
- e2e/AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (13)
- e2e/cleanup-pending.sh
- e2e/fuse-correctness-workload.sh
- e2e/git-workspace-smoke-test.sh
- e2e/fuse-write-perf-budget-test.sh
- e2e/fuse-performance-baseline.sh
- e2e/fuse-posix-fsx-gate.sh
- e2e/fuse-concurrency-stress.sh
- e2e/fuse-sqlite-correctness.sh
- e2e/smoke-all.sh
- e2e/portable-pack-unpack-e2e.sh
- e2e/README.md
- e2e/cli-smoke-test.sh
- e2e/provision-helper.sh
|
Review notes on
I checked the main large-file paths and they look covered: API, CLI, layer large fixtures/downloads, and |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/provision-helper.sh (1)
67-89: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate retry configuration before numeric comparisons.
If
DRIVE9_PROVISION_MAX_RETRIESis non-numeric,[ "$attempt" -ge "$max_retries" ]errors and retryable responses can loop forever. Validatemax_retriesandretry_sleeponce before the loop.Suggested patch
max_retries="${DRIVE9_PROVISION_MAX_RETRIES:-${REQUEST_MAX_RETRIES:-${CLI_MAX_RETRIES:-1}}}" retry_sleep="${DRIVE9_PROVISION_RETRY_SLEEP_S:-${REQUEST_RETRY_SLEEP_S:-${CLI_RETRY_SLEEP_S:-2}}}" + case "$max_retries" in + ''|*[!0-9]*|0) + echo "invalid DRIVE9_PROVISION_MAX_RETRIES/REQUEST_MAX_RETRIES/CLI_MAX_RETRIES: $max_retries" >&2 + return 1 + ;; + esac + case "$retry_sleep" in + ''|*[!0-9]*) + echo "invalid DRIVE9_PROVISION_RETRY_SLEEP_S/REQUEST_RETRY_SLEEP_S/CLI_RETRY_SLEEP_S: $retry_sleep" >&2 + return 1 + ;; + esac attempt=1🤖 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 `@e2e/provision-helper.sh` around lines 67 - 89, The retry logic in drive9_provision should validate the DRIVEN9_PROVISION_MAX_RETRIES and retry_sleep values before entering the loop, since the numeric test on attempt and max_retries can fail or misbehave when either variable is non-numeric. Add a pre-loop validation step in provision-helper.sh around the drive9_provision retry block to ensure both values are valid integers, and fail fast with a clear error if they are not.
🤖 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.
Inline comments:
In `@e2e/provision-helper.sh`:
- Around line 58-64: The two curl invocations in provision_helper.sh can block
indefinitely before the retry logic in the provisioning flow can run; update the
POST calls that assign to code to use bounded connect and total timeouts, and
make those timeout values configurable via environment variables for slower
endpoints. Apply the fix in the provision request path around the curl commands
so the retry loop can recover instead of hanging.
---
Outside diff comments:
In `@e2e/provision-helper.sh`:
- Around line 67-89: The retry logic in drive9_provision should validate the
DRIVEN9_PROVISION_MAX_RETRIES and retry_sleep values before entering the loop,
since the numeric test on attempt and max_retries can fail or misbehave when
either variable is non-numeric. Add a pre-loop validation step in
provision-helper.sh around the drive9_provision retry block to ensure both
values are valid integers, and fail fast with a clear error if they are not.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: af1ffd54-c34e-4086-af57-5ffe810ebf0f
📒 Files selected for processing (23)
e2e/AGENTS.mde2e/README.mde2e/_feature-matrix-runner.she2e/api-smoke-test-existing-key.she2e/api-smoke-test.she2e/cleanup-helper.she2e/cli-smoke-test.she2e/fuse-concurrency-stress.she2e/fuse-correctness-workload.she2e/fuse-crash-recovery-test.she2e/fuse-performance-baseline.she2e/fuse-posix-fsx-gate.she2e/fuse-smoke-test.she2e/fuse-sqlite-correctness.she2e/fuse-write-perf-budget-test.she2e/git-ops-smoke-test.she2e/git-workspace-smoke-test.she2e/journal-smoke-test.she2e/layer-fs-smoke-test.she2e/portable-pack-unpack-e2e.she2e/posix-permission-smoke-test.she2e/provision-helper.she2e/tmp-helper.sh
🚧 Files skipped from review as they are similar to previous changes (20)
- e2e/journal-smoke-test.sh
- e2e/git-workspace-smoke-test.sh
- e2e/git-ops-smoke-test.sh
- e2e/AGENTS.md
- e2e/portable-pack-unpack-e2e.sh
- e2e/fuse-performance-baseline.sh
- e2e/fuse-sqlite-correctness.sh
- e2e/fuse-smoke-test.sh
- e2e/fuse-concurrency-stress.sh
- e2e/fuse-crash-recovery-test.sh
- e2e/posix-permission-smoke-test.sh
- e2e/fuse-posix-fsx-gate.sh
- e2e/layer-fs-smoke-test.sh
- e2e/_feature-matrix-runner.sh
- e2e/fuse-write-perf-budget-test.sh
- e2e/api-smoke-test.sh
- e2e/fuse-correctness-workload.sh
- e2e/cli-smoke-test.sh
- e2e/cleanup-helper.sh
- e2e/README.md
| code=$(printf '%s' "$body" | curl -sS -o "$output_file" -w "%{http_code}" -X POST \ | ||
| -H "Content-Type: application/json" \ | ||
| --data-binary @- \ | ||
| "$base/v1/provision" || true) | ||
| else | ||
| code=$(curl -sS -o "$output_file" -w "%{http_code}" -X POST "$base/v1/provision" || true) | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add curl timeouts so retries can actually recover.
Both provision calls can hang indefinitely before the retry loop gets control. Add bounded connect/total timeouts, ideally env-configurable for slower hosted endpoints.
Suggested patch
+ local curl_connect_timeout="${DRIVE9_PROVISION_CONNECT_TIMEOUT_S:-10}"
+ local curl_max_time="${DRIVE9_PROVISION_MAX_TIME_S:-120}"
if [ -n "$body" ]; then
- code=$(printf '%s' "$body" | curl -sS -o "$output_file" -w "%{http_code}" -X POST \
+ code=$(printf '%s' "$body" | curl -sS --connect-timeout "$curl_connect_timeout" --max-time "$curl_max_time" -o "$output_file" -w "%{http_code}" -X POST \
-H "Content-Type: application/json" \
--data-binary `@-` \
"$base/v1/provision" || true)
else
- code=$(curl -sS -o "$output_file" -w "%{http_code}" -X POST "$base/v1/provision" || true)
+ code=$(curl -sS --connect-timeout "$curl_connect_timeout" --max-time "$curl_max_time" -o "$output_file" -w "%{http_code}" -X POST "$base/v1/provision" || true)
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| code=$(printf '%s' "$body" | curl -sS -o "$output_file" -w "%{http_code}" -X POST \ | |
| -H "Content-Type: application/json" \ | |
| --data-binary @- \ | |
| "$base/v1/provision" || true) | |
| else | |
| code=$(curl -sS -o "$output_file" -w "%{http_code}" -X POST "$base/v1/provision" || true) | |
| fi | |
| local curl_connect_timeout="${DRIVE9_PROVISION_CONNECT_TIMEOUT_S:-10}" | |
| local curl_max_time="${DRIVE9_PROVISION_MAX_TIME_S:-120}" | |
| if [ -n "$body" ]; then | |
| code=$(printf '%s' "$body" | curl -sS --connect-timeout "$curl_connect_timeout" --max-time "$curl_max_time" -o "$output_file" -w "%{http_code}" -X POST \ | |
| -H "Content-Type: application/json" \ | |
| --data-binary `@-` \ | |
| "$base/v1/provision" || true) | |
| else | |
| code=$(curl -sS --connect-timeout "$curl_connect_timeout" --max-time "$curl_max_time" -o "$output_file" -w "%{http_code}" -X POST "$base/v1/provision" || true) | |
| fi |
🤖 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 `@e2e/provision-helper.sh` around lines 58 - 64, The two curl invocations in
provision_helper.sh can block indefinitely before the retry logic in the
provisioning flow can run; update the POST calls that assign to code to use
bounded connect and total timeouts, and make those timeout values configurable
via environment variables for slower endpoints. Apply the fix in the provision
request path around the curl commands so the retry loop can recover instead of
hanging.
d717de3 to
8aef6cf
Compare
8aef6cf to
6c7418a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
e2e/provision-helper.sh (1)
56-64: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnbounded curl calls across the shared e2e helpers can hang indefinitely. None of these curl invocations set
--connect-timeout/--max-time; curl has no default operation timeout, so a slow or unresponsive hosted endpoint stalls the calling script (and, for the cleanup paths, blockssmoke-all.sh's EXIT trap from ever finishing).
e2e/provision-helper.sh#L56-L64: add--connect-timeout/--max-time(env-configurable) to both the body and no-body POST branches indrive9_provision_to_file.e2e/cleanup-helper.sh#L362-L393: add the same timeouts to theGET /v1/statuscall indrive9_cleanup_verify_deleted.e2e/cleanup-helper.sh#L420-L437: add the same timeouts to both DELETE branches indrive9_cleanup_delete_record.🤖 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 `@e2e/provision-helper.sh` around lines 56 - 64, Unbounded curl requests can hang indefinitely. Add shared, environment-configurable --connect-timeout and --max-time options to every request: both POST branches in drive9_provision_to_file (e2e/provision-helper.sh, lines 56-64), the GET /v1/status request in drive9_cleanup_verify_deleted (e2e/cleanup-helper.sh, lines 362-393), and both DELETE branches in drive9_cleanup_delete_record (e2e/cleanup-helper.sh, lines 420-437).
🤖 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.
Inline comments:
In `@e2e/layer-fs-smoke-test.sh`:
- Line 1023: Replace the hardcoded /tmp/ paths in the rollback layer tests with
drive9_e2e_tmp_path: update rollback_fuse_base_local at
e2e/layer-fs-smoke-test.sh lines 1023-1023, and wrap local_rb and log_rb at
lines 1038-1039 using the same helper while preserving their filenames.
---
Duplicate comments:
In `@e2e/provision-helper.sh`:
- Around line 56-64: Unbounded curl requests can hang indefinitely. Add shared,
environment-configurable --connect-timeout and --max-time options to every
request: both POST branches in drive9_provision_to_file
(e2e/provision-helper.sh, lines 56-64), the GET /v1/status request in
drive9_cleanup_verify_deleted (e2e/cleanup-helper.sh, lines 362-393), and both
DELETE branches in drive9_cleanup_delete_record (e2e/cleanup-helper.sh, lines
420-437).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 28c89c8d-a303-415f-bdf9-421726fcbaaf
📒 Files selected for processing (31)
e2e/AGENTS.mde2e/README.mde2e/_feature-matrix-runner.she2e/api-smoke-test-existing-key.she2e/api-smoke-test.she2e/cleanup-helper.she2e/cleanup-pending.she2e/cli-smoke-test.she2e/fuse-concurrency-stress.she2e/fuse-correctness-workload.she2e/fuse-crash-recovery-test.she2e/fuse-performance-baseline.she2e/fuse-posix-fsx-gate.she2e/fuse-smoke-test.she2e/fuse-sqlite-correctness.she2e/fuse-write-perf-budget-test.she2e/git-ops-smoke-test.she2e/git-workspace-smoke-test.she2e/journal-smoke-test.she2e/layer-fs-smoke-test.she2e/local-smoke.she2e/native-smoke-test.she2e/portable-pack-unpack-e2e.she2e/posix-permission-smoke-test.she2e/provision-helper.she2e/server-quota/quota-server-e2e.she2e/smoke-all.she2e/tmp-helper.she2e/verify-description-e2e.she2e/verify-description-tidb-zero-e2e.shscripts/verify_description.sh
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
♻️ Duplicate comments (1)
e2e/provision-helper.sh (1)
56-64: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnbounded curl calls across the shared e2e helpers can hang indefinitely. None of these curl invocations set
--connect-timeout/--max-time; curl has no default operation timeout, so a slow or unresponsive hosted endpoint stalls the calling script (and, for the cleanup paths, blockssmoke-all.sh's EXIT trap from ever finishing).
e2e/provision-helper.sh#L56-L64: add--connect-timeout/--max-time(env-configurable) to both the body and no-body POST branches indrive9_provision_to_file.e2e/cleanup-helper.sh#L362-L393: add the same timeouts to theGET /v1/statuscall indrive9_cleanup_verify_deleted.e2e/cleanup-helper.sh#L420-L437: add the same timeouts to both DELETE branches indrive9_cleanup_delete_record.🤖 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 `@e2e/provision-helper.sh` around lines 56 - 64, Unbounded curl requests can hang indefinitely. Add shared, environment-configurable --connect-timeout and --max-time options to every request: both POST branches in drive9_provision_to_file (e2e/provision-helper.sh, lines 56-64), the GET /v1/status request in drive9_cleanup_verify_deleted (e2e/cleanup-helper.sh, lines 362-393), and both DELETE branches in drive9_cleanup_delete_record (e2e/cleanup-helper.sh, lines 420-437).
🤖 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.
Inline comments:
In `@e2e/layer-fs-smoke-test.sh`:
- Line 1023: Replace the hardcoded /tmp/ paths in the rollback layer tests with
drive9_e2e_tmp_path: update rollback_fuse_base_local at
e2e/layer-fs-smoke-test.sh lines 1023-1023, and wrap local_rb and log_rb at
lines 1038-1039 using the same helper while preserving their filenames.
---
Duplicate comments:
In `@e2e/provision-helper.sh`:
- Around line 56-64: Unbounded curl requests can hang indefinitely. Add shared,
environment-configurable --connect-timeout and --max-time options to every
request: both POST branches in drive9_provision_to_file
(e2e/provision-helper.sh, lines 56-64), the GET /v1/status request in
drive9_cleanup_verify_deleted (e2e/cleanup-helper.sh, lines 362-393), and both
DELETE branches in drive9_cleanup_delete_record (e2e/cleanup-helper.sh, lines
420-437).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 28c89c8d-a303-415f-bdf9-421726fcbaaf
📒 Files selected for processing (31)
e2e/AGENTS.mde2e/README.mde2e/_feature-matrix-runner.she2e/api-smoke-test-existing-key.she2e/api-smoke-test.she2e/cleanup-helper.she2e/cleanup-pending.she2e/cli-smoke-test.she2e/fuse-concurrency-stress.she2e/fuse-correctness-workload.she2e/fuse-crash-recovery-test.she2e/fuse-performance-baseline.she2e/fuse-posix-fsx-gate.she2e/fuse-smoke-test.she2e/fuse-sqlite-correctness.she2e/fuse-write-perf-budget-test.she2e/git-ops-smoke-test.she2e/git-workspace-smoke-test.she2e/journal-smoke-test.she2e/layer-fs-smoke-test.she2e/local-smoke.she2e/native-smoke-test.she2e/portable-pack-unpack-e2e.she2e/posix-permission-smoke-test.she2e/provision-helper.she2e/server-quota/quota-server-e2e.she2e/smoke-all.she2e/tmp-helper.she2e/verify-description-e2e.she2e/verify-description-tidb-zero-e2e.shscripts/verify_description.sh
🛑 Comments failed to post (1)
e2e/layer-fs-smoke-test.sh (1)
1023-1023: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use
drive9_e2e_tmp_pathinstead of hardcoding/tmp/.The newly added rollback layer tests hardcode
/tmp/for their temporary files instead of usingdrive9_e2e_tmp_path. This bypasses the dynamic temporary directory logic ($DRIVE9_E2E_TMPDIR) introduced in this PR.
e2e/layer-fs-smoke-test.sh#L1023-L1023: wrap the path indrive9_e2e_tmp_path "drive9-layer-rollback-base-${ts}.txt"e2e/layer-fs-smoke-test.sh#L1038-L1039: wrap the paths forlocal_rbandlog_rbindrive9_e2e_tmp_path.📍 Affects 1 file
e2e/layer-fs-smoke-test.sh#L1023-L1023(this comment)e2e/layer-fs-smoke-test.sh#L1038-L1039🤖 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 `@e2e/layer-fs-smoke-test.sh` at line 1023, Replace the hardcoded /tmp/ paths in the rollback layer tests with drive9_e2e_tmp_path: update rollback_fuse_base_local at e2e/layer-fs-smoke-test.sh lines 1023-1023, and wrap local_rb and log_rb at lines 1038-1039 using the same helper while preserving their filenames.
1 participant
Summary
/v1/provisionhelper that preserves empty-body provisioning by default.public_key/private_keywhenDRIVE9_TIDBCLOUD_PUBLIC_KEYandDRIVE9_TIDBCLOUD_PRIVATE_KEYare set, with optional numericDRIVE9_TIDBCLOUD_SPENDING_LIMITmapped totidbcloud_spending_limit.tidb_cloud_nativecloud_provider/regionmetadata.DRIVE9_PUBLIC_KEY/DRIVE9_PRIVATE_KEYnames for context operations.Refs #676
Validation
bash -n e2e/*.sh e2e/server-quota/*.shgit diff --checkDRIVE9_BASE=https://aws-ap-southeast-1.drive9.ai CLI_SOURCE=build bash e2e/cli-smoke-test.sh→RESULT: 94 passed, 0 failed, 0 skipped, 94 totalDRIVE9_BASE=https://aws-ap-southeast-1.drive9.ai CLI_SOURCE=build bash e2e/smoke-all.sh→PASS=6 FAIL=0The live smoke run used credentials from the local EC2 env file and did not print key material.
Summary by CodeRabbit
DRIVE9_E2E_TMPDIRand changed defaultFUSE_MOUNT_ROOTfrom/tmptoDRIVE9_E2E_TMPDIR.