Fix/3362 grpc timestamp access #3366

remo-lab · 2025-12-11T18:28:11Z

Describe the changes that are made

This PR fixes an issue where the replay service always accessed HTTPReq.Timestamp from the first test case, even when the first test case was a gRPC request. This resulted in incorrect or zero timestamps being used during mock matching.

To address this cleanly, the timestamp selection logic has been extracted into a dedicated helper function and both replay paths now use this unified function.

What was the problem?
When replaying test cases, the code assumed that the first entry in testCases was always an HTTP test. For gRPC-first scenarios, this caused:
an incorrect zero-value timestamp
inconsistent backdate calculations
potential nil dereferences
incorrect mock matching behavior
This bug is tracked in Issue #3362.

What’s changed?

Extracted getBackdateTimestamp helper (replay.go: 102–112):
A new reusable function now handles selecting the correct timestamp based on the test case kind (HTTP or GRPC_EXPORT).
This eliminates duplicated logic and ensures both replay paths use the same behavior.
Updated both replay paths to use the helper:
Docker Compose path (line ~747)
Non-Docker path (line ~838)
Both now simply call:
backdate := getBackdateTimestamp(testCases)
This guarantees consistent timestamp handling.
Added comprehensive unit tests (replay_test.go)
All tests now call the real getBackdateTimestamp() function — no duplicated logic.

The following cases are covered:
gRPC-first test case (original bug scenario)
HTTP-first test case (regression test)
Empty test case list
Nil test case entry

To run:
go test -v ./pkg/service/replay -run TestGetBackdateTimestamp
i have also provided the screenshot ahter running test below.

Links & References

Why this fix matters:
Ensures correct timestamps are used for both HTTP and gRPC test cases
Eliminates the previous zero-value timestamp bug
Removes duplicated logic and makes replay behavior consistent
Adds reliable test coverage for all edge cases
Improves maintainability and safety for future changes

Files Modified:
pkg/service/replay/replay.go (timestamp logic extracted + call sites updated)
pkg/service/replay/replay_test.go (new tests added)

Closes: #[issue number that will be closed through this PR]

NA (if very small change like typo, linting, etc.)

🔗 Related PRs

NA

🐞 Related Issues

NA

📄 Related Documents

NA

What type of PR is this? (check all applicable)

Added e2e test pipeline?

👍 yes
🙅 no, because they aren't needed
🙋 no, because I need help

Added comments for hard-to-understand areas?

👍 yes
🙅 no, because the code is self-explanatory

Added to documentation?

📜 README.md
📓 Wiki
🙅 no documentation needed

Are there any sample code or steps to test the changes?

[✔️ ] 👍 yes, i have added a test file
🙅 no, because it is not needed

Self Review done?

[✔️ ] ✅ yes
❌ no, because I need help

Any relevant screenshots, recordings or logs?

🧠 Semantics for PR Title & Branch Name

Please ensure your PR title and branch name follow the Keploy semantics:

📌 PR Semantics Guide
📌 Branch Semantics Guide

Examples:

PR Title: fix: patch MongoDB document update bug
Branch Name: feat/#1-login-flow (You may skip mentioning the issue number in the branch name if the change is small and the PR description clearly explains it.)

Additional checklist:

[✔️ ] Have you read the Contributing Guidelines on issues?
[✔️ ] Have you followed the PR Semantics guide for naming this PR?
[✔️ ] Have you followed the Branch Semantics guide for naming your branch?

Copilot

Pull request overview

This PR fixes a bug where the replay service incorrectly accessed HTTPReq.Timestamp from the first test case, even when that test case was a gRPC request, resulting in zero or incorrect timestamps during mock matching. The fix extracts timestamp selection logic into a dedicated helper function that correctly handles both HTTP and gRPC test cases.

Introduced getBackdateTimestamp helper function to safely extract timestamps based on test case Kind
Updated both Docker Compose and non-Docker replay paths to use the new helper
Added comprehensive unit tests covering gRPC-first, HTTP-first, empty, and nil edge cases

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
pkg/service/replay/replay.go	Adds `getBackdateTimestamp` helper function and updates both replay code paths (lines 747, 838) to use it instead of directly accessing `testCases[0].HTTPReq.Timestamp`
pkg/service/replay/replay_test.go	Adds four unit tests validating the helper function with gRPC-first, HTTP-first, empty, and nil test case scenarios

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

Copilot · 2025-12-11T18:31:09Z

pkg/service/replay/replay.go

+func getBackdateTimestamp(testCases []*models.TestCase) time.Time {
+	var backdate time.Time
+	if len(testCases) > 0 && testCases[0] != nil {
+		if testCases[0].Kind == models.HTTP {
+			backdate = testCases[0].HTTPReq.Timestamp
+		} else if testCases[0].Kind == models.GRPC_EXPORT {
+			backdate = testCases[0].GrpcReq.Timestamp
+		}
+	}
+	return backdate
+}


The helper function getBackdateTimestamp is missing documentation. Consider adding a comment that explains its purpose, parameters, and return value. For example: "getBackdateTimestamp extracts the timestamp from the first test case based on its Kind (HTTP or GRPC_EXPORT). Returns zero time if the test cases are empty, nil, or have an unrecognized Kind."

Copilot · 2025-12-11T18:31:10Z

pkg/service/replay/replay.go

+		if testCases[0].Kind == models.HTTP {
+			backdate = testCases[0].HTTPReq.Timestamp
+		} else if testCases[0].Kind == models.GRPC_EXPORT {
+			backdate = testCases[0].GrpcReq.Timestamp
+		}


The function currently only handles HTTP and GRPC_EXPORT kinds. While these are the only test case kinds currently used in the codebase, consider adding a default case that logs a warning if an unexpected Kind is encountered. This would make the code more defensive and easier to debug if new test case kinds are added in the future.

Copilot · 2025-12-11T18:31:10Z

pkg/service/replay/replay_test.go

+		t.Errorf("Expected zero time for nil test case, got %v", backdate)
+	}
+}
+


There's an extra blank line at the end of the file. Remove this trailing newline to maintain consistency with Go code style.

Suggested change

AkashKumar7902 · 2025-12-12T07:06:10Z

@remo-lab please address copilot comments

AkashKumar7902

please address the review comments

AkashKumar7902 · 2025-12-12T07:07:36Z

pkg/service/replay/replay.go

+			backdate = testCases[0].GrpcReq.Timestamp
+		}
+	}
+	return backdate


the branching is too much, can we shorten it? also what happens if backdate is returned as it is (means it doesn't go on the if condition of 104), shouldn't we log a warning ?

also improve the function name, it should be short and to the point

AkashKumar7902 · 2025-12-12T11:14:46Z

Pls link the issue corresponding to this pr

remo-lab · 2025-12-12T14:21:28Z

HI @AkashKumar7902 ,Thanks for the feedback! I’ve made the requested updates and refined the implementation:

Simplified the branching by switching to a switch statement, which removes the nested conditions and makes the logic easier to read.

1)Improved the function name to backdate so it’s shorter and more aligned with Go naming conventions.

2)Converted it into a method on Replayer so we can use the instance logger (r.logger) for warnings.

3)Added warning logs for all edge cases (empty test cases, unknown kind, zero timestamps) to make unexpected flows more visible.

4)Added a doc comment explaining what the method does and how it handles HTTP and gRPC test cases.

5)Updated the tests to use r.backdate() directly and removed the trailing newline that Copilot flagged.

Everything compiles cleanly and all tests are passing. Let me know if you’d like anything else improved — happy to adjust!

…ar first. Signed-off-by: remo-lab <remopanda7@gmail.com>

Signed-off-by: remo-lab <remopanda7@gmail.com>

…ests Signed-off-by: remo-lab <remopanda7@gmail.com>

remo-lab · 2025-12-15T22:20:44Z

Hi @AkashKumar7902 ,I have Implemented the suggested changes for this PR.”

remo-lab · 2025-12-17T00:23:40Z

Hi @AkashKumar7902, I’ve implemented the suggested changes for this PR.
Whenever you get time, could you please review it again? Thanks!

remo-lab · 2025-12-21T04:25:48Z

Hi @Sarthak160 @gouravkrosx , can you please approve this pr?

remo-lab · 2025-12-23T02:17:51Z

HI @AkashKumar7902 @gouravkrosx @Sarthak160 could you please approve this pr?

Copilot AI review requested due to automatic review settings December 11, 2025 18:28

remo-lab requested review from Sarthak160 and gouravkrosx as code owners December 11, 2025 18:28

Copilot started reviewing on behalf of remo-lab December 11, 2025 18:28 View session

Copilot AI reviewed Dec 11, 2025

View reviewed changes

remo-lab force-pushed the fix/3362-grpc-timestamp-access branch from 9f2c6d2 to 393886d Compare December 11, 2025 18:46

remo-lab mentioned this pull request Dec 11, 2025

[bug]: Incorrect Timestamp Access for gRPC Test Cases #3362

Open

1 task

AkashKumar7902 reviewed Dec 12, 2025

View reviewed changes

Mayanknishad9 mentioned this pull request Dec 12, 2025

[feature]: add grpc fuzzer pipeline #3130

Closed

1 task

remo-lab added 3 commits December 12, 2025 19:52

fix(replay): use correct timestamp for gRPC test cases when they appe…

0afc214

…ar first. Signed-off-by: remo-lab <remopanda7@gmail.com>

fix(replay): correct gRPC timestamp handling and add unit tests

8c4130a

Signed-off-by: remo-lab <remopanda7@gmail.com>

refactor(replay): simplify backdate logic, add warnings, and update t…

9c0d0bd

…ests Signed-off-by: remo-lab <remopanda7@gmail.com>

remo-lab force-pushed the fix/3362-grpc-timestamp-access branch from 71723f3 to 9c0d0bd Compare December 12, 2025 14:23

remo-lab mentioned this pull request Dec 15, 2025

fix(record): resolve record service deadlock and data race #3371

Open

25 tasks

Merge branch 'main' into fix/3362-grpc-timestamp-access

c67e560

Uh oh!

Fix/3362 grpc timestamp access #3366

Are you sure you want to change the base?

Fix/3362 grpc timestamp access #3366

Conversation

remo-lab commented Dec 11, 2025

Describe the changes that are made

Links & References

🔗 Related PRs

🐞 Related Issues

📄 Related Documents

What type of PR is this? (check all applicable)

Added e2e test pipeline?

Added comments for hard-to-understand areas?

Added to documentation?

Are there any sample code or steps to test the changes?

Self Review done?

Any relevant screenshots, recordings or logs?

🧠 Semantics for PR Title & Branch Name

Additional checklist:

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

AkashKumar7902 commented Dec 12, 2025

Uh oh!

AkashKumar7902 left a comment

Choose a reason for hiding this comment

Uh oh!

AkashKumar7902 Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

AkashKumar7902 Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

AkashKumar7902 commented Dec 12, 2025

Uh oh!

remo-lab commented Dec 12, 2025

Uh oh!

remo-lab commented Dec 15, 2025

Uh oh!

remo-lab commented Dec 17, 2025

Uh oh!

remo-lab commented Dec 21, 2025

Uh oh!

remo-lab commented Dec 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants