ING-1308: Add configurable timeout for Data API KV operations

[ingenthr]

• Introduce a Data API–specific KV timeout (dapiKvTimeout) to gateway configuration, parsed as a Go-style duration
string and threaded through gateway.Config → dapiimpl.NewOptions → DataApiServer.
• Insure the timeout is configurable with clamping and logging: ignore non-positive values, coerce values
< 1s to 1s, and values > 120s down to 120s, ensuring safe bounds for all Data API KV operations.

[Copilot]

Pull request overview

This PR introduces a configurable timeout for Data API key-value operations to improve control over operation durations and prevent indefinite hangs.

Changes:

• Added DapiKvTimeout configuration field to gateway.Config and ReconfigureOptions with a default of 120 seconds
• Implemented timeout application across all Data API KV operations (CRUD, subdoc, locking, counter, binary, and touch operations)
• Added validation logic with clamping (1s minimum, 120s maximum) and logging for invalid timeout values

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
gateway/gateway.go	Added DapiKvTimeout field to Config and ReconfigureOptions; applied default timeout handling in Run method
gateway/dapiimpl/dapiimpl.go	Threaded timeout configuration through to DataApiServer via ReconfigureKvTimeout
gateway/dapiimpl/server_v1/dataapi.go	Added kvTimeout field and ReconfigureKvTimeout method with validation/clamping logic
gateway/dapiimpl/server_v1/dataapi_crud.go	Applied timeout to GetDocument, CreateDocument, UpdateDocument, and DeleteDocument operations
gateway/dapiimpl/server_v1/dataapi_subdoc.go	Applied timeout to LookupInDocument and MutateInDocument operations
gateway/dapiimpl/server_v1/dataapi_locking.go	Applied timeout to LockDocument and UnlockDocument operations
gateway/dapiimpl/server_v1/dataapi_counter.go	Applied timeout to IncrementDocument and DecrementDocument operations
gateway/dapiimpl/server_v1/dataapi_binary.go	Applied timeout to AppendToDocument and PrependToDocument operations
gateway/dapiimpl/server_v1/dataapi_touch.go	Applied timeout to TouchDocument operation
gateway/test/dapi_crud_test.go	Added test to verify GetDocument endpoint functions correctly with timeout logic

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Magic number time.Second for minimum timeout should be extracted as a named constant (e.g., MinKvTimeout). This makes the constraint explicit and easier to adjust if requirements change.

[ingenthr]

I don't agree in this case. A system constant seems reasonable here, but open to disagreement.

[ingenthr]

@Westwooo should I expect both of those to run? I didn't see anything wrong with my specific changes. Can you help?

[ingenthr]

The jira-validator seems to be owing to the branch not being available. Does this mean I need to push my branch here versus my own repo before PR? Or maybe we just need to update this job. Sayth co-pilot:

The failing job 62025923053 in the jira-validator workflow encountered this error:

error: pathspec 'ING-1308' did not match any file(s) known to git

This occurred during the "Prep" step, specifically while running:

git checkout -q ${{ github.event.pull_request.head.ref }}

The value 'ING-1308' appears to be the pull request branch name, but it does not exist in the checked-out repository context at this point in the workflow. The checkout step is using:

with:
fetch-depth: 0
ref: '${{ github.event.pull_request.base.ref }}'

This only checks out the base branch, not the head (feature) branch for the PR. As a result, git cannot find the branch 'ING-1308'.

Solution:
Modify the checkout step to fetch both the base and head branches. Use the following update in your .github/workflows/jira-validator.yml file:

- uses: actions/checkout@v2
  with:
    fetch-depth: 0
    # Remove 'ref' so both base and head branches are available

Then, ensure you fetch all branches before the git checkout in the "Prep" step:

- name: Prep
  run: |
    git fetch origin "+refs/heads/*:refs/remotes/origin/*"
    git checkout -q ${{ github.event.pull_request.head.ref }}
    BASE_BRANCH=${{ github.event.pull_request.base.ref }}
    echo "BASE_BRANCH=$BASE_BRANCH" | tee -a $GITHUB_ENV

This will allow git to find and check out the PR’s source branch, resolving the error.

[ingenthr]

For the 8.1 tests, is it just flaky? Sayth co-pilot:

The job failed during integration tests, specifically in TestGatewayOps with several errors in subtests related to Upsert (e.g., ValueTooLargeCompressed, CollectionMissing, ScopeMissing, BucketMissing, BadCredentials, BadAPIVersion, and others).

Key failures from the logs:

• TestUpsert/ReadOnlyCreds and TestUpsert/NoPermissions: "skipping due to no dino cluster id"
• TestUpsert/BucketMissing and similar: “Bucket unknown, triggering manual update” and “Bucket still unknown, erroring”
• TestUpsert/BadAPIVersion: "failed to parse api version header" due to an invalid date string

Recommended solutions:

1. Ensure test setup provides required cluster ID and valid test buckets/scopes/collections where necessary. These cases are being skipped or fail as resources do not exist.

   • Add setup/teardown logic to provision required Couchbase resources or update test configuration so required resources are present before test execution.

2. For BadAPIVersion failures:

   • Update the test to use a valid API version header or update parsing logic in the gRPC interceptor to properly handle or reject invalid input gracefully.
   • Example code (for stricter error handling):

     // apiversion/grpcinterceptor.go
     ver, err := time.Parse("2006-01-02", headerValue)
     if err != nil {
         // Log error and return appropriate error response
         // instead of panicking or causing test failures
     }

3. For skipped tests due to missing cluster id:

   • Confirm integration test environment variables are correctly set, especially for dependent services (dino cluster, Couchbase server).

4. For Bucket/Scope/Collection “missing” cases:

   • Seed tests with expected resources, or update error responses for these cases if the intent is to validate error handling.

Summary: The integration test environment is missing required buckets, scopes, or credentials. Fix the test setup to provision these resources and review authentication and version parsing code paths for more robust error handling.

[Westwooo]

nit: is it worth renaming this to maxDapiKvTimeout, since we do not allow the timeout to be set above this?

[Westwooo]

Should we remove this if since we perform validation on the DapiKvTimeout being > 0 inside ReconfigureKvTimeout?