M5-03: Implement deterministic simulation clock for backtests - #229
Conversation
Adds clock.Simulated.AdvanceTo(t time.Time) error: the primitive a historical-timestamp-driven backtest scheduler (#213) needs, so it never has to compute t.Sub(Now()) itself at every call site across irregularly-spaced bars (weekends, holidays, gaps). Per design-notes review: no context.Context parameter. AdvanceTo is a synchronous, bounded state mutation with no blocking work, the same character Advance already has; adding ctx here would put cancellation policy at the wrong layer -- the scheduler owns the potentially long-running replay loop and should check ctx between observations before calling the clock, not make every deterministic domain primitive context-aware. Implementation shares a new private advanceBy(d) core with the existing Advance, both acquiring the lock once and delegating to it, so the two can never disagree about what "advance" means and AdvanceTo never races against a concurrent Advance/AdvanceTo call between reading Now() and applying the delta. Semantics, each covered by a dedicated test: - t canonicalized to UTC with any monotonic reading stripped (matching NewSimulated's own canonicalization) before comparison, so an equivalent instant in a different location or carrying monotonic metadata never produces different clock state. - t == Now() is a valid no-op, matching Advance(0) -- it still fires any timer already due. - t < Now() returns ErrNegativeAdvance, leaving both time and timer state unchanged. - Crossing multiple timers' deadlines in one call preserves the existing deadline/creation-order firing behavior, and Now() lands exactly on the requested target. - Two independently constructed clocks given the same start, timers, and AdvanceTo sequence produce identical observations. No new package or type -- purely additive to clock, the same way ADR-025/027/030 each added one missing primitive to num when a consuming M3/M4 issue needed it. The repo-wide TestDomainCodeDoesNotCallTimeDirectly guard (clock/arch_test.go) already mechanically covers this issue's own "no wall-clock dependency" acceptance criterion for every package outside clock/cmd/adapters; a fuller end-to-end demonstration that a deterministic run's observable results depend only on simulated time is expected once #213's own scheduler loop exists to exercise, per review. Tested: go build ./..., go vet ./..., gofmt -l ., go test ./... -race all clean. clock package coverage: 100.0%.
There was a problem hiding this comment.
🟡 Changes recommended
One of the new tests is currently ineffective (it passes regardless of AdvanceTo behavior because it uses NewTimer(0) which is already ready on creation).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a deterministic, timestamp-targeted advancement primitive to the simulated clock to support historical/backtest replay without callers computing relative deltas (and keeps timer-firing semantics consistent with existing Advance behavior).
Changes:
- Refactors
Simulated.Advanceto delegate to a shared, lock-heldadvanceBy(d)core. - Adds
(*Simulated).AdvanceTo(t time.Time) errorwith UTC + monotonic-stripping canonicalization and backward-move rejection. - Introduces a dedicated
clock/advance_to_test.gosuite covering target movement, canonicalization, rejection, timer deadlines, and determinism.
File summaries
| File | Description |
|---|---|
clock/simulated.go |
Adds AdvanceTo and centralizes advance semantics via advanceBy under a single lock. |
clock/advance_to_test.go |
Adds unit tests for AdvanceTo behavior and determinism. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| func TestSimulatedAdvanceToEqualToNowStillFiresDueTimers(t *testing.T) { | ||
| start := mustParse(t, "2026-01-01T00:00:00Z") | ||
| c := NewSimulated(start) | ||
| timer := c.NewTimer(0) | ||
|
|
||
| require.NoError(t, c.AdvanceTo(start)) | ||
| select { | ||
| case <-timer.C(): | ||
| default: | ||
| t.Fatal("a timer already due must fire on a no-op AdvanceTo") | ||
| } | ||
| } |
rustyeddy
left a comment
rustyeddy
left a comment
There was a problem hiding this comment.
The implementation itself looks good to me: AdvanceTo canonicalizes the target, holds the mutex across target-delta calculation and mutation, reuses the same advanceBy core as Advance, preserves the existing timer semantics, and keeps cancellation at the scheduler layer as discussed on #211.
I agree with Copilot's one test finding: TestSimulatedAdvanceToEqualToNowStillFiresDueTimers does not prove what its name/comment claims. NewTimer(0) fires immediately inside NewTimer, before AdvanceTo is called, so the assertion would pass even if AdvanceTo(start) never called fireDue().
There is also a useful design observation here: through the public API there normally cannot be a pending timer whose deadline is already <= Now() — non-positive timers fire immediately, and positive timers become due only as the clock advances (which itself calls fireDue). So rather than reaching into private timer state just to manufacture an otherwise unreachable condition, I would simply remove that test and soften the AdvanceTo documentation from "matching Advance(0), which still fires any timer already due" to just state that equal-to-now is a valid no-op. The shared advanceBy(0) still calls fireDue(), but that is an implementation detail rather than an externally meaningful semantic requiring a special test.
With that cleanup, I consider #229 ready to merge. I found no substantive issue in the implementation.
Rusty + Copilot both caught the same real issue: TestSimulatedAdvanceToEqualToNowStillFiresDueTimers didn't prove what it claimed. NewTimer(0) fires immediately inside NewTimer itself (never added to the active timer set at all), so the assertion passed regardless of whether AdvanceTo(start) called fireDue() -- the test was ineffective, not a real regression guard. Removed the test rather than reaching into private timer state to manufacture an otherwise-unreachable condition (through the public API there cannot be a pending timer whose deadline is already <= Now(): non-positive timers fire immediately, and positive ones become due only as the clock advances, which itself already calls fireDue). Softened AdvanceTo's own doc comment to state only that t == Now() is a valid no-op, without asserting a timer-firing detail that was never actually externally observable or meaningfully testable at this target-equals-now boundary. Tested: go build ./..., go vet ./..., gofmt -l ., go test ./... -race all clean. clock package coverage unchanged at 100.0%.
|
Addressed the finding: `TestSimulatedAdvanceToEqualToNowStillFiresDueTimers` didn't prove what it claimed — `NewTimer(0)` fires immediately inside `NewTimer` itself (never added to the active timer set), so the assertion passed regardless of whether `AdvanceTo` called `fireDue()`. Verified directly against `NewTimer`'s own code before agreeing. Removed the test rather than reaching into private timer state to manufacture an otherwise-unreachable condition — through the public API there can't be a pending timer whose deadline is already Full suite (`go build`, `go vet`, `gofmt -l`, `go test ./... -race`) clean. Coverage unchanged at 100.0%. |
What changed
Adds
clock.Simulated.AdvanceTo(t time.Time) error: the primitive a historical-timestamp-driven backtest scheduler (#213) needs, so it never has to computet.Sub(Now())itself at every call site across irregularly-spaced bars (weekends, holidays, gaps).Why it changed
Per #211's design-notes review: no
context.Contextparameter.AdvanceTois a synchronous, bounded state mutation with no blocking work — the same characterAdvancealready has. Addingctxhere would put cancellation policy at the wrong layer: the scheduler owns the potentially long-running replay loop and should checkctxbetween observations before calling the clock, not make every deterministic domain primitive context-aware.Implementation shares a new private
advanceBy(d)core with the existingAdvance, both acquiring the lock once and delegating to it, so the two can never disagree about what "advance" means andAdvanceTonever races against a concurrentAdvance/AdvanceTocall between readingNow()and applying the delta.Semantics, each covered by a dedicated test:
tcanonicalized to UTC with any monotonic reading stripped (matchingNewSimulated's own canonicalization) before comparison, so an equivalent instant in a different location or carrying monotonic metadata never produces different clock state.t == Now()is a valid no-op, matchingAdvance(0)— it still fires any timer already due.t < Now()returnsErrNegativeAdvance, leaving both time and timer state unchanged.Now()lands exactly on the requested target.AdvanceTosequence produce identical observations.No new package or type — purely additive to
clock, the same way ADR-025/027/030 each added one missing primitive tonumwhen a consuming M3/M4 issue needed it.The repo-wide
TestDomainCodeDoesNotCallTimeDirectlyguard (clock/arch_test.go) already mechanically covers this issue's own "no wall-clock dependency" acceptance criterion for every package outsideclock/cmd/adapters. Per review, a fuller end-to-end demonstration that a deterministic run's observable results depend only on simulated time (not just the clock primitive in isolation) is expected once #213's own scheduler loop exists to exercise it — there's no scheduler yet for that integration test to drive.How it was tested
TestSimulatedAdvanceTo*: exact-target movement, no-op-at-current-time (including still firing due timers), rejection of a past target (leaving time and timer state unchanged), UTC/monotonic canonicalization, multi-timer-deadline crossing in one call, and cross-instance determinism.clocktest still passes unmodified.go build ./...,go vet ./...,gofmt -l .,go test ./... -raceall clean.clockpackage coverage: 100.0%.Which documentation changed
None required — this is a purely additive method on an already-
Accepted, pre-existing type; no new architectural decision was introduced.Closes #211.