fix(engine): put the feed's timeline offset back on the decision time - #136
Merged
Conversation
#131 moved -output_ts_offset from the moment a switch is decided to the moment the replacement feed actually starts, on the argument that teardownFeed blocks for as long as the outgoing process takes to exit, so a pre-teardown time would start the incoming feed behind where the outgoing one's timestamps had reached. That argument reads well and is wrong. Measured, twelve runs of the failover suite per configuration: main (post-#131) 3/12 backwards DTS b5ff0e2 (pre-#131) 0/12 main, selector.go reverted 0/12 main, offset reverted, backoff kept 0/12 main, offset kept, backoff reverted 3/12 The offset carries it and the backoff change is innocent, so the backoff fix stays: feedAt genuinely does want the post-teardown time, or a feed that fails to start respawns on every 500ms sweep. The bug the old comment described was never observed. Pre-#131 measures 0/12, so the backwards step "from a slow teardown" was derived from reading the code rather than from watching it, and fixing it caused the failure it claimed to prevent. The mechanism behind the real one is not yet established -- #126 keeps that -- but the direction is, and shipping the inverted version while the mechanism is worked out is not defensible. Also removed: feedOffset, which was never called from production -- startFeed computes the offset inline -- and the three tests that covered this change. Two of them read selector.go as TEXT and asserted it contained a particular line, which is the defect issue #107 is open about, reproduced here in the engine. The third exercised the dead helper. All three passed while the behaviour regressed from 0/12 to 3/12, and their green was part of what I used to justify merging. acceptance-failover is the guard that actually caught this.
|
There was a problem hiding this comment.
Pull request overview
This PR reverts the selector feed’s -output_ts_offset timing back to the decision time (pre-teardown) after measurements showed the post-teardown offset change from #131 increased backwards-DTS occurrences in acceptance-failover. It keeps the post-teardown timestamp for feedAt to preserve respawn backoff behavior.
Changes:
- In
ensureFeed, pass the decision-timenowintostartFeedagain so the feed timeline offset is derived from the decision point rather than post-teardown time. - Keep
feedAtas a post-teardown timestamp (startedAt := time.Now()) to prevent respawn loops when teardown is slow and the replacement feed fails to start. - Remove
feedOffsetand the associated seam tests (including the source-text assertions described as defective in the PR description).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/engine/selector.go | Reverts timeline offset to decision time; expands in-code commentary documenting measured behavior; retains post-teardown feedAt for backoff correctness. |
| internal/engine/feed_seam_test.go | Removes seam-related unit tests (including brittle source-text assertions) and the unused feedOffset helper coverage. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1149
to
1156
| // startedAt is still read, because feedAt genuinely does want the later time: | ||
| // it is what feedRespawn measures against, and a pre-teardown value means the | ||
| // backoff has already expired when the feed starts, so a feed that fails to | ||
| // start respawns on every 500ms sweep. That half was measured innocent -- | ||
| // 0 of 12 with it kept and the offset reverted. | ||
| startedAt := time.Now() | ||
| feed := e.startFeed(s, want, upstream, silenceSig, startedAt) | ||
| feed := e.startFeed(s, want, upstream, silenceSig, now) | ||
|
|
rainmanjam
added a commit
that referenced
this pull request
Aug 9, 2026
…ot claim it The 0.6.0 notes named two failover fixes. One of them was reverted in #136 after measurement showed it caused the backwards DTS it was written to prevent, so within this release it is a net no-op and claiming it would be describing work nobody receives. The revert is recorded instead, with the numbers, because a reader who sees #131 in the log should be able to find out what happened to it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#131 moved
-output_ts_offsetfrom the moment a switch is decided to the moment the replacement feed actually starts. I wrote that change, argued it from the code, and it caused the failure it claimed to prevent.Twelve runs of
acceptance-failoverper configuration:main(post-#131)b5ff0e2(pre-#131)main,selector.gorevertedmain, offset reverted, backoff keptmain, offset kept, backoff revertedThe offset carries it; the backoff change is innocent and stays —
feedAtgenuinely wants the post-teardown time, or a feed that fails to start respawns on every 500 ms sweep.The bug the old comment described was never observed. Pre-#131 measures 0/12, so the "backwards step from a slow teardown" was derived from reading rather than from watching, and the fix for it inverted a working seam. The real mechanism is still unestablished — #126 keeps that — but the direction is now measured, and the comment in
ensureFeedrecords the numbers so this is not "fixed" again from first principles.Tests removed, not replaced in kind
feedOffsetwas never called from production —startFeedcomputes the offset inline — so the helper and its test went with it.The other two read
selector.goas text and asserted it contained a particular line. That is the defect issue #107 is open about, reproduced in the engine. All three passed while behaviour went 0/12 → 3/12, and I cited their green when merging #131.acceptance-failoveris the guard that actually caught this. A seam test worth having has to start processes and read real DTS; a grep over source cannot.12-run verification on this branch is running.
https://claude.ai/code/session_01HeLrWaDmsNeeNSbHQfEofX