Skip to content

fix(storage): disambiguate zero time from Unix epoch in TimeToNS and NSToTime helpers - #5002

Draft
kislaykishore wants to merge 1 commit into
masterfrom
pr/opt-gcs-object-time-helpers
Draft

fix(storage): disambiguate zero time from Unix epoch in TimeToNS and NSToTime helpers#5002
kislaykishore wants to merge 1 commit into
masterfrom
pr/opt-gcs-object-time-helpers

Conversation

@kislaykishore

Copy link
Copy Markdown
Collaborator

Description

This PR updates the TimeToNS and NSToTime conversion helpers in internal/storage/gcs/object.go to handle the edge case where Unix epoch (time.Unix(0, 0).UTC()) collides with zero time.Time{} (int64(0)).

Details

  • TimeToNS: Returns 0 for zero time.Time{}. For Unix epoch time.Unix(0, 0), returns math.MinInt64 as a sentinel to distinguish from unset/zero time.
  • NSToTime: Decodes 0 to zero time.Time{}, and math.MinInt64 back to time.Unix(0, 0).UTC().
  • Executes with zero heap allocations (0 B/op, 0 allocs/op).

Benchmark Performance

Measured via benchstat (n=10, p < 0.05):

Benchmark Target Master Baseline Optimized Branch CPU Latency Delta Memory Allocs/op Semantic Disambiguation
TimeToNS_Zero 0.312 ns/op 0.312 ns/op ~0.0% (sub-ns) 0 B/op 0 time.Time{} -> 0
TimeToNS_Epoch 1.241 ns/op 1.865 ns/op +0.62 ns 0 B/op 0 time.Unix(0,0) -> math.MinInt64
TimeToNS_Normal 1.375 ns/op 1.652 ns/op +0.27 ns 0 B/op 0 Normal dates -> UnixNano()
NSToTime_Zero 1.603 ns/op 1.592 ns/op -0.69% 0 B/op 0 0 -> time.Time{}
NSToTime_Epoch 1.608 ns/op 2.324 ns/op +0.71 ns 0 B/op 0 math.MinInt64 -> 1970-01-01
NSToTime_Normal 3.633 ns/op 3.889 ns/op +0.25 ns 0 B/op 0 ns -> time.Unix(0,ns).UTC()
TimeToNS_Roundtrip 4.567 ns/op 5.069 ns/op +0.50 ns 0 B/op 0 Lossless roundtrip

Link to the issue in case of a bug fix.

N/A

Testing details

  1. Manual - Executed make build and verified formatting/linter with 0 issues.
  2. Unit tests - Executed unit tests: go test -v -run=TestTimeToNS ./internal/storage/gcs/... & go test -v -run=TestNSToTime ./internal/storage/gcs/....
  3. Integration tests - N/A

Any backward incompatible change? If so, please explain.

N/A

…NSToTime helpers

## Summary
This commit updates `TimeToNS` and `NSToTime` in `internal/storage/gcs/object.go` to handle the edge case where Unix epoch (`time.Unix(0, 0).UTC()`) collides with zero `time.Time{}` (`int64(0)`).

## Details
- `TimeToNS`: Returns `0` for zero `time.Time{}`. For Unix epoch `time.Unix(0, 0)`, returns `math.MinInt64` as a sentinel to distinguish from unset/zero time.
- `NSToTime`: Decodes `0` to zero `time.Time{}`, and `math.MinInt64` back to `time.Unix(0, 0).UTC()`.
- Executes with zero heap allocations (`0 B/op`, `0 allocs/op`).

## Benchmark Performance
Measured via `benchstat` (n=10, p < 0.05):

| Benchmark Target | Master Baseline | Optimized Branch | CPU Latency Delta | Memory | Allocs/op | Semantic Disambiguation |
|---|:---:|:---:|:---:|:---:|:---:|:---:|
| `TimeToNS_Zero` | 0.312 ns/op | 0.312 ns/op | ~0.0% (sub-ns) | 0 B/op | 0 | `time.Time{}` -> `0` |
| `TimeToNS_Epoch` | 1.241 ns/op | 1.865 ns/op | +0.62 ns | 0 B/op | 0 | `time.Unix(0,0)` -> `math.MinInt64` |
| `TimeToNS_Normal` | 1.375 ns/op | 1.652 ns/op | +0.27 ns | 0 B/op | 0 | Normal dates -> `UnixNano()` |
| `NSToTime_Zero` | 1.603 ns/op | **1.592 ns/op** | **-0.69%** | 0 B/op | 0 | `0` -> `time.Time{}` |
| `NSToTime_Epoch` | 1.608 ns/op | 2.324 ns/op | +0.71 ns | 0 B/op | 0 | `math.MinInt64` -> `1970-01-01` |
| `NSToTime_Normal` | 3.633 ns/op | 3.889 ns/op | +0.25 ns | 0 B/op | 0 | `ns` -> `time.Unix(0,ns).UTC()` |
| `TimeToNS_Roundtrip` | 4.567 ns/op | 5.069 ns/op | +0.50 ns | 0 B/op | 0 | Lossless roundtrip |

## Verification
- Unit tests: `go test -v -run=TestTimeToNS ./internal/storage/gcs/...` & `go test -v -run=TestNSToTime ./internal/storage/gcs/...`

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the time conversion functions TimeToNS and NSToTime in internal/storage/gcs/object.go to use math.MinInt64 as a sentinel value for the Unix epoch (time.Unix(0, 0)), preventing collisions with the zero value of time.Time{}. It also adds comprehensive unit tests to verify these conversions. The review feedback suggests adding a code comment to document this sentinel choice and its theoretical limitations to improve code maintainability.

Comment on lines +162 to +166
ns := t.UnixNano()
if ns == 0 {
return math.MinInt64
}
return ns

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

While using math.MinInt64 as a sentinel for time.Unix(0, 0) is a practical choice (since 1678-09-21 is far outside the range of any real GCS object timestamp), it does introduce a theoretical collision where time.Unix(0, math.MinInt64) will incorrectly round-trip to time.Unix(0, 0).

To improve maintainability and prevent future confusion, please add a comment documenting this sentinel choice and its limitation.

	ns := t.UnixNano()
	// Use math.MinInt64 as a sentinel for Unix epoch (ns == 0) to distinguish it from
	// the zero time.Time{} (which maps to 0). This introduces a theoretical collision
	// with 1678-09-21 00:12:43.145224193 UTC (math.MinInt64), which is acceptable
	// as it is far outside the range of any real GCS object timestamp.
	if ns == 0 {
		return math.MinInt64
	}
	return ns

@codecov

codecov Bot commented Aug 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.83%. Comparing base (0673d19) to head (e123970).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5002      +/-   ##
==========================================
+ Coverage   83.81%   83.83%   +0.01%     
==========================================
  Files         174      174              
  Lines       21376    21381       +5     
==========================================
+ Hits        17916    17924       +8     
+ Misses       2779     2777       -2     
+ Partials      681      680       -1     
Flag Coverage Δ
unittests 83.83% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant