refactor(time): put a clock seam in the timestamp cache - #255
Conversation
time.go read the wall clock and built its ticker inline, so the only way a test could observe the cache was to wait for real time to pass. time_test.go did exactly that -- 3.35 s of sleeps across four tests -- and paid for it twice: the waits were the slowest thing in the package's suite, and every assertion had to stay loose enough to survive a loaded CI box. "Timestamp should have updated after 1+ seconds" cannot tell a correct update from one that is a second out. Route both reads through two unexported package variables, now and newTicker. Production keeps time.Now and time.NewTicker; tests install a fake and step it. Nothing is added to the public interface, Timestamp touches neither variable, and the defaults make this a no-op for every caller. newTicker yields a channel and a stop function rather than a *time.Ticker, because a Ticker the runtime did not build cannot stand in for one -- its Stop panics. The tick source is created in StartTimeStampUpdater rather than inside the goroutine, so it exists by the time Start returns, and it is released before close(done) rather than after, so a returning Stop leaves no ticker still live. The tests no longer sleep. In isolation they go from 3.362 s to 0.007 s, and the root package under -race -shuffle from ~12.5 s to ~9.2 s (three runs each, same machine). Ticks are delivered synchronously: advance sends twice, and since the updater takes ticks one at a time, the second send can only be accepted after the store that followed the first -- which is what makes the read after it exact rather than eventual. Exactness is the point. Three mutants that master's suite accepts now fail: tick path stores now()+1 Test_TimeStampUpdater tick source never released on Stop Test_StopTimeStampUpdater Start's idempotence guard removed Test_StartTimeStampUpdater_Idempotent The first is the off-by-one second the ±2 s tolerance was blind to by construction. The other two were never covered at all: StartTimeStampUpdater's doc comment has always promised that "only one updater runs at a time", and nothing checked it. Those two guards were the coverage gap -- Start 93.3% and Stop 87.5% on master, both 100% here. Benchmark_CalculateTimestamp keeps the real clock and its ±2 s tolerance, which is the cache's own lag rather than slack. It is unchanged: benchstat n=10 with samples interleaved between the revisions puts every row at ~, the stdlib default rows included. That is the expected result, since Timestamp's body is untouched and the clock is read once per second, never on the measured path. No README rows change: the catalog is darwin/arm64, this machine is linux/amd64, and the measurement says there is nothing to record. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K7Z8sz85kpAwQKxoeFBv7i
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #255 +/- ##
==========================================
+ Coverage 93.61% 93.78% +0.17%
==========================================
Files 38 38
Lines 2426 2429 +3
==========================================
+ Hits 2271 2278 +7
+ Misses 135 133 -2
+ Partials 20 18 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Warning Review limit reachedNext included review available in 55 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (11)
🧰 Additional context used📓 Path-based instructions (1)Do not move exported functions or packages to `internal/` merely because they have no non-test in-module callers; this library intentionally exposes helpers for downstream modules.📄 CodeRabbit inference engine (AGENTS.md) Files:
🪛 ast-grep (0.45.3)time.go[warning] 64-64: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values. (integer-overflow-narrowing-conversion-go) time_test.go[warning] 93-93: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values. (integer-overflow-narrowing-conversion-go) [warning] 112-112: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values. (integer-overflow-narrowing-conversion-go) [warning] 129-129: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values. (integer-overflow-narrowing-conversion-go) [warning] 142-142: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values. (integer-overflow-narrowing-conversion-go) [warning] 147-147: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values. (integer-overflow-narrowing-conversion-go) 🔇 Additional comments (2)
📝 WalkthroughWalkthroughThe timestamp updater now uses replaceable clock functions and creates its ticker before starting the updater goroutine. Tests use a fake clock and atomic counters to verify timestamp updates, idempotent starts, stops, and restarts without sleeping. ChangesTimestamp updater clock control
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to The deterministic timestamp updater preserves lifecycle ownership and test isolation, with no concrete merge-blocking issue identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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. A rabbit reads each line, Comment |
Every test in the file swaps newTicker for a fake, so the real one -- the two statements that build a time.Ticker and hand back its channel and Stop -- ran nowhere under coverage. codecov/patch caught it on #255 at 62.5% of the diff, and go tool cover -func had not: it does not list anonymous package-level closures, so the per-function report read 100% while time.go:26 was dead. The gap was not cosmetic. Nothing proved the production adapter delivers a live channel or that the stop it returns works, which is exactly the part of the seam that a fake cannot vouch for. Test_NewTicker asserts both against the real clock with a millisecond period, the one real-time wait left in the file, under a timeout that fails loudly instead of hanging CI. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K7Z8sz85kpAwQKxoeFBv7i
|
Three notes on the bot feedback, and one correction to the description above.
Worth saying that my own pre-push check missed this. I used The fix is not coverage padding. That closure was the one part of the seam a fake cannot vouch for: whether the real tick source delivers a live channel, and whether the Correction to the description: it says there is no sleep or polling left in the file. That is no longer exactly true — CodeRabbit's Docstring Coverage warning (44.44% vs 80%): not taking it. The nine functions it scored are four test functions, three one-line For the record on the ast-grep Generated by Claude Code |
⚡ 2 benchmarks faster (up to 1.97x)
d23bb8b vs master@5482477 · 2304/2304 results compared · retest: 2/2 improvements reproduced · noise-aware thresholds · full results · |
Description
time.goread the wall clock and built its ticker inline, so the only way a test could observe the cache was to wait for real time to pass.time_test.godid exactly that — 3.35 s of sleeps across four tests — and paid for it twice: the waits were the slowest thing in the package's suite, and every assertion had to stay loose enough to survive a loaded CI box."Timestamp should have updated after 1+ seconds"cannot tell a correct update from one that is a second out.Both reads now go through two unexported package variables,
nowandnewTicker. Production keepstime.Nowandtime.NewTicker; the tests install a fake and step it. Nothing is added to the public interface,Timestamptouches neither variable, and the defaults make this a no-op for every caller.newTickeryields a channel and a stop function rather than a*time.Ticker, because aTickerthe runtime did not build cannot stand in for one — itsStoppanics. Two small behaviour fixes came along with that: the tick source is created inStartTimeStampUpdaterrather than inside the goroutine, so it exists by the timeStartreturns; and it is released beforeclose(done)rather than after, so a returningStopno longer leaves a ticker still live.Fixes # (issue)
No linked issue — this came out of the same module-boundary review of this repository as #252.
Changes introduced
Benchmarks:
Benchmark_CalculateTimestampis unchanged, and this was measured rather than assumed.benchstat,n=10, samples interleaved between compiled test binaries for the two revisions:fiber~(p=0.529)default~(p=0.631)fiber_asserted~(p=0.404)default_asserted~(p=0.118)Every row
~, withB/opandallocs/opidentical across all samples. The two stdlibdefaultrows are controls — they are untouched code and came back~in both directions, so the run is not noise-dominated. That is the expected outcome:Timestamp's body is unchanged, and the clock is read once per second, never on the measured path.Measured on
linux/amd64, Go 1.26. NoREADME.mdrows change: the catalog atREADME.md:17-20isdarwin/arm64(Apple M2 Pro,-12rows) and this machine is a 4-corelinux/amd64Xeon, so regenerating there would put amd64 numbers under an arm64 header — the defect that still sits atREADME.md:400-403. The measurement above says there is nothing to record in any case.Documentation Update: doc comments only. The new comments state what the seam is for, why
newTickerreturns a channel plus a stop function instead of a*time.Ticker, why the tick source is created outside the goroutine, and why it is released beforeclose(done).Benchmark_CalculateTimestampgains a one-line note that it deliberately stays on the real clock.Changelog/What's New: not applicable — this repository has no changelog file; release notes come from commit subjects.
Migration Guide: not needed. The public API is untouched and both new variables are unexported.
API Alignment with Express: not applicable — no Express-facing surface is involved.
API Longevity:
Timestamp,StartTimeStampUpdaterandStopTimeStampUpdaterkeep their exact signatures and semantics. The seam is internal, which is the point: the clock became substitutable without anything new appearing in the interface.Examples: not applicable for a test-observability change.
Speed
-race -shuffle=onThree runs each on the same machine. There is no sleep and no polling left: ticks are delivered synchronously.
advancesends twice, and since the updater handles ticks one at a time, the second send can only be accepted after the store that followed the first — which is what makes the read after it exact rather than eventual.Tests
Determinism is the means; exact assertions are the point. Three mutants that
master's suite accepts now fail. Each was applied in both worlds and run against that world's own tests:master's testsnow()+1Test_TimeStampUpdaterStopTest_StopTimeStampUpdaterStart's idempotence guard removedTest_StartTimeStampUpdater_IdempotentStartdoes not publish before returningThe first is exactly the off-by-one second the ±2 s tolerance was blind to by construction. The other two had no coverage at all —
StartTimeStampUpdater's doc comment has always promised that "only one updater runs at a time", and nothing checked it. Those two guards were the coverage gap:StartTimeStampUpdaterStopTimeStampUpdaterTimestampBenchmark_CalculateTimestampdeliberately keeps the real clock and its ±2 s tolerance: there the tolerance is the cache's own lag againsttime.Now, not slack in an assertion.timerTestMustays. The updater is package-global state regardless of the seam, so these tests still cannot run in parallel with each other — only the waiting went away, not the shared state.Type of change
The template has no "test quality" or "refactor" option; this is closest. There is no behaviour change for callers and no performance claim — the benchmarks above exist to show the seam costs nothing.
Checklist
Followed the inspiration of the Express.js framework for new functionalities— not applicable; no new functionality and no Express-facing API.close(done)) and the two-send synchronization in the test helper, none of which is self-evident from the code.Updated the documentation in the— not applicable; this repository has no/docs/directory/docs/.Start's idempotence, verified by mutation rather than by coverage alone.make testgreen (757 tests,-race -shuffle=on),go vetclean,gofumptclean,golangci-lint0 issues.One incidental find, not fixed here
README.md:446-447records12 B/op / 2 allocsfor the two asserted sub-benchmarks, but bothmasterand this branch now measure0 B/op / 0 allocs— those rows predate the allocation fix thatcheckTimeStamp's own comment describes. It is pre-existing, it is in the arm64 catalog I cannot regenerate on this machine, and it is out of scope for this diff. Flagging it rather than silently leaving it unmentioned.Commit formatting
Single commit, conventional-commit style, matching the prevailing convention in this repository's history:
refactor(time): put a clock seam in the timestamp cache🤖 Generated with Claude Code
https://claude.ai/code/session_01K7Z8sz85kpAwQKxoeFBv7i
Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Tests