Harden Google self-heal follow-ups: atomic session saves, fast cookie-save retry, paced-repair observability#151
Merged
Conversation
…-save retry, paced-repair observability (#148 review) Non-blocking polish from PR #148's cross-family review (sol). #148 made maybePersistRotatedCookies rewrite session.json every ~5 minutes, which turned two latent nits into real exposure: - internal/client/session.go: SaveSession now writes a same-dir 0600 temp file, fsyncs, and renames over session.json instead of os.WriteFile in place. A crash mid-save can no longer truncate the only copy of the paired Google/WhatsApp/Signal credentials (which costs a manual re-pair). Mirrors googlecookies.UpdateSessionCookies, plus fsync. Regression-tested with a concurrent writers+reader test that fails against the old implementation (torn JSON read). - internal/client/events.go: a failed rotated-cookie save now retries after interval/10 (~30s) instead of silently waiting the full ~5-min throttle, still bounded so a persistent failure can't write per event. lastCookieHash stays untouched on failure (now asserted), so the retry still sees the pending rotation. Test proven against the old advance-before-save semantics. - Paced-repair observability: the fast-cookie-revocation signal from the durability adjudication (theory A) was test-only. A delayed repair now logs a warn with wait/since_last_repair/paced_total, and the count is published as google.repairs_paced in /api/status and MCP get_status (omitted when 0; client/demo modes report 0 — only the transport-owning daemon builds a repairer). Runbook documents the check and the healthy-is-zero expectation. All touched packages pass with -race. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
Non-blocking polish from PR #148's cross-family review (sol findings). #148 made
maybePersistRotatedCookiesrewritesession.jsonevery ~5 minutes, which turned two latent nits into real exposure; the third item makes the fast-revocation detector visible in production.1. Atomic
SaveSession(internal/client/session.go)SaveSessiondid a directos.WriteFileofsession.json— a crash mid-write could truncate the file holding the paired Google/WhatsApp/Signal credentials, and losing it costs a manual re-pair. It now writes a unique same-dir temp file (0600), fsyncs, and renames over the target, mirroringgooglecookies.UpdateSessionCookies(plus fsync, so a power loss can't commit the rename ahead of the contents). Dir 0700 / file 0600 perms preserved; a unique temp name (not a fixed.tmp) keeps concurrent savers from installing each other's partial writes.Test: concurrent writers + torn-read detector, verified to FAIL against the old
os.WriteFileimplementation (unmarshal: unexpected end of JSON input); plus a failed-save-keeps-previous-session test asserting no debris and intact prior contents.2. Failed cookie-save retry (internal/client/events.go)
The throttle timestamp advanced before the save attempt, so a transient
SaveSessionfailure wasn't retried for a full ~5-min interval. Resolution (the "OR" branch from the review, made explicit): the throttle still advances up front — these events arrive per message, so an unthrottled failure path would rewrite the session on each one — but a failed save pulls the next attempt in tointerval/10(~30s), andlastCookieHashstays untouched on failure so the retry still sees the pending rotation. The deterministic marshal-failure path deliberately keeps the full interval (commented).Test: blocked-session-path harness proving one attempt per window, retry at interval/10 (not per event, not a full interval), and that the eventual successful save persists the rotation the failures skipped — verified to FAIL against the old advance-before-save semantics.
3. Paced-repair observability (cmd/google_supervisor.go → status surfaces)
pacedCount— the "cookies are being revoked within minutes" detector from the durability adjudication (theory A) — was test-only. Now:wait,since_last_repair,min_interval,paced_total./api/status:google.repairs_paced(omitempty — absent in the healthy 0 case). Wired viaapp.SetGoogleRepairPaceCounter; only the transport-owning daemon builds a repairer, so client/demo modes report 0. MCP client mode inherits it for free throughRawStatusdaemon-truth passthrough.get_status:Repairs paced: Nline, only when non-zero.jqone-liner and the healthy-is-zero/climbing-means-churn interpretation, next to the heal-cycle description.Testing
go build ./...clean; touched packages (internal/client,internal/app,internal/tools,internal/web,cmd,internal/googlecookies) pass with-race -count=1internal/clientor the supervisor — no overlap with in-flight work🤖 Generated with Claude Code