fix(chatv3): stop and join the standing ticker on process close - #1256
Closed
santoshkumarradha wants to merge 2 commits into
Closed
santoshkumarradha wants to merge 2 commits into
santoshkumarradha wants to merge 2 commits into
Conversation
startStandingTicks launched a process-global endless ticker through a package sync.Once, and closeAll never stopped or joined it. After Close returned the ticker was still armed, so the next pass created lock, item, run and log files under <home>/v3/standing (and appended standing.log), a writer that outlived the process and left <home>/v3 non-empty for a caller trying to remove it. The ticker is now owned by the v3Process: a stop and a done channel taken under p.mu, a select loop that exits on stop, and closeAll calls stopStandingTicks, which closes stop and waits on done so an in-flight pass finishes before Close returns. The package sync.Once is gone, so a second process opened later in the same test binary starts its own ticker; the old standingTicks gauge is cleared on stop. A real-close-path test opens a process, lets it tick, closes it, removes the standing root, waits several intervals, and asserts nothing was recreated, then shows a second process can start and stop its own ticker.
…it out closeAll joined the ticker by waiting on done, but the pass in flight ran under context.WithTimeout(context.Background(), TickWindow), which stop did not cancel, so a person who quit while a pass was out could wait up to the 120s window. Quitting must not scale with background work. The ticker now holds one context that stop cancels: runStandingTick derives the pass ceiling from it (a child of that context and the 120s window), and a guarded watcher cancels it when stop closes, so an in-flight pass ends at once and the join is prompt. The 120s stays the pass's own upper bound when nobody is quitting. A cancelled pass leaves no half-written file: standing writes are temp+rename or single short appends, and Tick checks ctx between steps, so it stops at the next boundary having only completed atomic writes. A failing-first test holds a pass in flight, closes, and asserts Close returns promptly and the pass saw its ctx cancelled; ordering is by channels with a generous sanity bound. The zero-writes-after-close test stays.
Member
Author
|
Landed on |
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
2 participants
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.
What
startStandingTickslaunched a process-global endless ticker via a packagesync.Once, andcloseAllnever stopped or joined it. After Close returned, the next pass created files under<home>/v3/standing(and appendedstanding.log), a writer outliving the process that left<home>/v3non-empty (the<tmp>/v3 directory not emptyleftover class).Fix
The ticker is owned by
v3Process: stop and done channels taken underp.mu, a select loop that exits on stop, andcloseAllcallsstopStandingTicks(closes stop, waits on done). The packagesync.Onceis removed, so a later process in the same test binary starts its own ticker. Behavior preserved: same interval, silence, at-most-once per process, ErrHeld silence.Interaction with #1250's settle-at-stop path (asked at review)
No adverse interaction.
closeAllcallsstopStandingTicksAFTERstopPoolErrandsand BEFORE the agent-close loop where #1250's settle path runs.stopStandingTickstakesp.muonly briefly to lift the stop/done channels, then waits on<-doneOUTSIDE the lock, so it neither runs concurrently with nor shares a lock with the main-agent settle path; the standing pass builds a separate standing session, not one ofp.agents. Worst case, closeAll waits one bounded standing TickWindow for an in-flight pass, which is the intended no-writer-outlives-close guarantee.Verification (Spark, off eae7055, all via scripts/one-suite.sh for the heavy suite)
TestCloseAllJoinsStandingTickerAndLaterProcessCanStart-count=8: pass (zero writes under<home>/v3/standingafter Close; second process starts its ticker).cmd/codeaf+internal/standing-count=5under the one-suite lock: ok (459s, 0 FAIL).go build ./...;gofmt -l ./cmd ./internal(empty);go run ./cmd/codeaf-changes check;go test ./internal/guard ./internal/namelaw(ok). No em dash.Flake note
One earlier cmd/codeaf failure occurred on the prior base via a direct
go testat load ~26 (not through one-suite); its goroutine dump pointed at chat.go:4622, the design-intentional detached worker-leaf flush goroutine, not this change's standing/close path. It did not recur across 5 one-suite runs on eae7055, and this change's own close-path tests pass 5x.