Skip to content

fix(go): cancel shared work that no caller waits for - #274

Open
lan17 wants to merge 13 commits into
mainfrom
fix/go-cancel-abandoned-flights
Open

lan17 wants to merge 13 commits into
mainfrom
fix/go-cancel-abandoned-flights

Conversation

@lan17

@lan17 lan17 commented Nov 12, 2026 •

Copy link
Copy Markdown
Owner

Problem

Since #255, a coalesced flight runs its shared work on context.WithoutCancel, and each caller waits on its own context. Nothing ends the shared work when every caller has left:

  • A source that honors its context keeps running after every caller has left: until SourceTimeout (60 s by default), or forever with NoTimeout. Each abandoned flight holds two goroutines and the source's resources. Later callers for the same key join the abandoned flight and fail at their own deadline with no new source call.
  • The abandoned flight still fills the caches when its source returns. With a short future buffer, that frame can be stamped after an invalidation and serve the pre-invalidation value.

Found while re-checking main against go/v0.27.0; no issue was filed.

Change

Bug fix. No design record: shared work keeps #255's rule that one caller leaving does not fail the others, and its budget stays the one docs/coalescing.md describes (a caller timeout does not by itself cancel shared work; SourceTimeout bounds it). The maintainer chose this shape in the session.

  • Each flight owns a cancel function and counts its waiters, the leader and its followers. A waiter whose context ends leaves; the last one cancels the shared work. A request-local flight that waits on a process flight is one waiter of it, so the cancellation passes through both levels.
  • A source runs at most once per key at a time. A cancelled flight stays published until its work returns, and shared work waits for its source until SourceTimeout, not for its own context. Callers still leave on their own contexts. So a source that ignores its context is not started again by each new caller. A caller that joins cancelled work that is still running gets that work's outcome: its value, stale recovery, or its source timeout. If the cancelled work ends with another error, the caller starts or joins fresh work with its own context.
  • Work that no caller waits for any more fills no cache: not Redis, the local layer, the request memo, or a coalesced dark-shadow fill. For a flight, a caller waits while the flight has a waiter; outside a flight, while the caller's context is live. A dark-shadow fill of uncoalesced work still outlives its caller.
  • A caller whose context has already ended starts and joins no shared work.
  • go-parity.json pins for go/cache.go, go/engine.go and go/shadow.go are refreshed; the TypeScript mapping is unchanged. Mutants M20, M29, M34 and M37 are re-anchored to the new code with the same faults.

Not changed, as documented and as in TypeScript:

  • A hung source that honors its context on a hot key holds the key until SourceTimeout, because a hot key always has a waiting caller. Set SourceTimeout at or below the callers' deadlines to bound it.
  • A stalled Redis write is bounded by the Redis client's own timeouts; NewRedisClient sets 250 ms.

Cost: each coalesced call allocates a cancelable context, 2 more allocations (benchmarks below). Coalescing off is unchanged.

Ports

Port Status
TypeScript N/A: the fault is in Go's flight lifetime from #255, which changed only Go
Go done
Rust N/A: same reason
Python N/A: same reason

Validation

Head: 3a51d8b. Since 0301209, found by mutant M26 and by review: a follower retries only when the cancelled flight has left the registry; a caller that joins cancelled work that is still running gets that work's outcome (stale recovery, the fallback event, or the inherited source timeout) instead of starting the source again; and the no-fill check for a remote write runs right before the write.

  • New tests at 0301209 (-race -count=3): 16 in go/flight_lifetime_test.go and 2 in go/internal/testsuite/context_test.go, all pass. Each fails on main (d9618cd) or on the earlier branch commit that it guards, except TestLoaderDeadlineErrorDoesNotRestartWork, which guards against a wrong retry and passes on both.
  • make check-go, make formal-go and make audit at 0301209: pass.
  • make integration-go at 24fe78d: pass (Redis, Valkey, Cluster and TypeScript interoperability).
  • make mutations-go MUTATION_ONLY=M20,M29,M34,M37 while re-anchoring: all four detected by the generated and portable suites.
  • Stale-on-error scenarios (20 cases and 2 cross-layer cases, local Redis, -race -count=3) at 0301209: same results as main.
  • formal-full.yaml in mutations mode at 0301209: run 43157274950 failed one Go shard: mutant M26 (a settled flight left in the registry) made TestFollowerOfCancelledWorkRetriesWithItsOwnContext spin instead of fail. Every other shard passed. Fixed in c6f9136.
  • make mutations-go MUTATION_ONLY=M20,M25,M26 at c6f9136: all detected; M26 now by every suite (ordinary 25, generated 2,797, fixed 45, portable 2,842). M34 detected (generated 1,199, fixed 10, portable 1,209). make check-go and make formal-go at c6f9136: pass.
  • make check-go, make formal-go and make audit at 2d2c3f7: pass. New test TestCallerLeavingDuringEncodeStopsTheWrite fails at c6f9136 (1 write, want 0) and passes at 2d2c3f7.
  • formal-full.yaml in mutations mode at c6f9136: run 43165881350, cancelled for the review fix.
  • formal-full.yaml in mutations mode at 2d2c3f7: run 43167821481, pass (replay in all four ports, symbolic lane, and the TypeScript, Go and Rust mutation campaigns).
  • make check-go, make formal-go and make audit at 26cdf05: pass. New test TestContextFollowerOfCancelledWorkInheritsItsOutcome fails at 2d2c3f7 (the follower starts the source again and fails at its own deadline) and passes at 26cdf05.
  • formal-full.yaml in mutations mode at 26cdf05: run 43301134435, cancelled (the head changed).
  • 217dd66 (found by re-check): the check that decides whether a follower inherits the work's error now runs inside the panic guard. At 26cdf05 it ran after the guard, so a source error whose Unwrap panics (a typed-nil pointer error) crashed the process when the only caller had left. The no-fill check also runs before the tracked TTL clamp, so a skipped write emits no tracked_ttl_clamped event. New test TestPanickingErrorFromAbandonedWorkDoesNotCrash crashes at 26cdf05 (engine.go:262) and passes at 217dd66. make check-go, make formal-go, make audit and the lifetime tests under -race -count=3 pass at 217dd66.
  • 22a9daa (found by re-check): 217dd66 moved the no-fill check before the tracked TTL clamp, so a slow observer of the clamp event let the last caller leave and the abandoned work still wrote to Redis. The early check stays, so a skipped write emits no clamp event, and a second check runs right before the write. New test TestCallerLeavingDuringTheTTLClampEventStopsTheWrite fails at 217dd66 (1 write) and passes at 22a9daa (0 writes). make check-go, make formal-go, make audit and the lifetime tests under -race -count=3 pass at 22a9daa.
  • PR CI at 22a9daa: every check passes.
  • 97f52ad: docs only. Maintainer decision: a caller that arrives after explicit invalidation and joins a flight whose earlier callers have all left can get the value read before the invalidation, as for a flight whose callers still wait (same on main). docs/coalescing.md now says that the flight stays open until its source settles or times out. make audit and the docs prose and link checks (scripts/check-docs) pass; the full make docs build needs Rust and runs in PR CI.
  • 3a51d8b (found by the final review round on 26cdf05..97f52ad): with the error classification inside the work's recovered closure, a panic in the work skipped it, so a caller that joined cancelled work got the panic instead of starting fresh work. The work and the timeout check now each run under their own recover. New test TestFollowerOfCancelledWorkThatPanicsRetries fails at 97f52ad (panic error after 1 source call) and passes at 3a51d8b. TestCallerLeavingDuringEncodeStopsTheWrite now also checks that a write skipped before the tracked TTL clamp emits no clamp event; it fails when the first check is removed. make check-go, make formal-go, make audit and the lifetime and internal/testsuite tests under -race -count=3 pass at 3a51d8b.
  • PR CI at 3a51d8b: every check passes.
  • formal-full.yaml in mutations mode at 3a51d8b: run 43329879068, pass, 37 of 37 jobs (replay in all four ports, symbolic lane, and the TypeScript, Go and Rust mutation campaigns).

Scenario runs, external test modules with local Redis 8 and an in-memory remote, main (d9618cd) against 0301209:

Scenario main this branch
500 keys, hung source, 100 ms caller deadline: live loaders 1 s after callers returned 500 (+1,000 goroutines) 0 (+0)
NoTimeout, 1,000 keys, 20 ms deadline: live loaders after 5 s 1,000 (+2,000 goroutines) 0 (+0)
Later caller on that key, 50 ms deadline deadline exceeded, 0 source calls value, 1 source call
SourceTimeout 2 s, 10,000 keys, 1 ms deadline: live sources at the end 1,676 0
Source ignores its context and runs 5 s, hot key at 1 per ms, 50 ms deadline, 3 s: loaders started 1 1
Healthy 80 ms source, callers at 1 per ms with 50 ms deadlines, 3 s 2,647 ok, 1 source call 2,614 ok, 1 source call
Stale recovery, SourceTimeout 100 ms, leader 60 ms, follower 140 ms follower served stale follower served stale at 101 ms
Abandoned source returns later (local-only, request memo, remote): next call abandoned value cached fresh value
Invalidation while a cancelled leader's flight runs (future buffer 100 ms or 0): stale reads in 4 cases 4 of 4 0 of 4 (v0.27.0: 0 of 4)
Hot key, source hangs 2 s then answers, 50 ms deadline, 8 s no success (default SourceTimeout); successes from second 3 with SourceTimeout 3 s same as main

In the invalidation runs, two cases are unchanged by this PR: a follower that leaves while the leader still waits (stale on main, this branch and v0.27.0, because a live caller keeps the work), and an uncoalesced dark-shadow fill (see Follow-ups).

Benchmarks (GOMAXPROCS=2, one CPU, 6 runs, benchstat, main → this branch):

Benchmark sec/op allocs/op
Local hit, coalescing on 3.70 µs → 3.93 µs 18 → 20
Local hit, coalescing on, caller deadline 3.74 µs → 3.91 µs (not significant) 18 → 20
Remote hit, coalescing on 13.6 µs → 14.6 µs 52 → 55
Miss, coalescing on 17.7 µs → 18.3 µs (not significant) 62 → 67
Local hit, remote hit and miss, coalescing off unchanged unchanged

Not run: none.

Follow-ups

  • An uncoalesced dark-shadow fill still runs on the shadow job's own context after its caller leaves, so it can write after an invalidation when the future buffer does not cover the shadow budget. Re-reading the watermark before the shadow SET fixes it but changes the read counts in the formal model, so it needs its own change.
  • A coalesced leader no longer returns a value that the loader returned together with an error; v0.27.0 did. To check separately.
  • When a cancellation lands inside an encode or a Redis write, the cancelled work still reports a serialization or cache-write error.
  • Every coalesced call still starts a leader goroutine (from fix(go): give each caller its own context wait and detach shared work #255), which costs more than this change on tiny hits.

Since #255 a coalesced flight runs on context.WithoutCancel, so a source that honors its context kept running after every caller had left: until SourceTimeout, or forever with NoTimeout. Later callers joined the abandoned flight and failed at their own deadlines.

Each flight now owns a cancel function and counts its waiters. When the last waiter leaves, the flight is cancelled and removed from the map, so a later caller starts fresh work. One waiter leaving, including the leader, still does not fail the others.
A hot key always has a waiting caller, so cancelling on the last waiter alone left a hung source holding the key until SourceTimeout. Shared work now keeps the leader's deadline, as when the leader ran it inline, while the leader's cancellation still ends only its own wait. A follower whose shared work ran out of the leader's deadline starts or joins fresh work with its own context. A stalled cache write on the same context now also ends at that deadline.

A caller whose context has already ended starts and joins no shared work.
@lan17
lan17 marked this pull request as ready for review November 12, 2026 03:46
@codecov-commenter

codecov-commenter commented Nov 12, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.72131% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.77%. Comparing base (f2b4f86) to head (3a51d8b).

Files with missing lines Patch % Lines
go/engine.go 96.61% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #274      +/-   ##
==========================================
+ Coverage   94.75%   94.77%   +0.02%     
==========================================
  Files          84       84              
  Lines       13124    13168      +44     
  Branches      805      805              
==========================================
+ Hits        12435    12480      +45     
+ Misses        623      622       -1     
  Partials       66       66              
Flag Coverage Δ
go 93.90% <96.72%> (+0.15%) ⬆️
python 93.60% <ø> (ø)
rust 94.78% <ø> (ø)
typescript 97.21% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@lan17 lan17 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agent: Review of head 24fe78d in report mode. Lanes: correctness, concurrency, tests, reliability, performance, simplicity and holistic. The holistic lane made an independent pass first, then audited the findings. This is a comment review. It does not approve and does not request changes.

Result: 3 findings (2 medium, 1 low). No blocker and no high. The simplicity, reliability, performance and concurrency lanes found no issue. The code does what the description says: the last waiter cancels and unpublishes the flight, the leader's deadline bounds the work, followers retry after expiry, and a caller that already left starts no work. The 8 new tests pass at the head with go test -race -count=3 on Go 1.27.1.

ID Severity Tag Location Fix
F1 medium V go/engine.go:252 Set expired from the leader's deadline, not from the timer state: f.expired = hasDeadline && !time.Now().Before(deadline). Keep the repro as a test.
F2 medium V go/flight_lifetime_test.go:123 Add a nested test that cancels with no deadline.
F3 low V go/engine.go:230 Add the mixed-deadline cost to Trade-offs. No code change.

Tag V: we ran it or read the code at the head. F1 and F2 have inline comments with a repro.

F3 (low, V): the Trade-offs list does not show the cost for callers that outlive a short leader.
Scenario: a hot key gets a caller every 1 ms with a 30 ms deadline and one caller with a 1 s deadline. The source honors ctx and takes 100 ms. The remote always misses. We ran 30 synctest trials on each commit.

  • At 24fe78d: the long caller succeeds 30 of 30. Its latency is 155 to 522 ms (most 156 to 250 ms). The source gets 3 to 30 calls per trial.
  • At f2b4f86 (main): 96 ms in all 30 trials, with 1 to 2 source calls.
  • On go/v0.27.0 a follower takes the leader's error, so the long caller would fail.

So the branch is between v0.27.0 and main, as the approved design expects. Suggested fix: add to Trade-offs that a caller with a long deadline behind short-deadline leaders can wait through several leader deadlines, and that each expiry starts a new source call.

Questions (not findings):

  • A follower that retries emits a second coalesced event. The old flight is already unpublished, so GetCoalescingState is not affected.
  • A source that ignores ctx and hangs: each leader deadline now starts a new source call while the old calls keep running. On main one call ran until SourceTimeout. This is the same shape as go/v0.27.0.
  • Stale-on-error with a leader deadline shorter than SourceTimeout: the flight ends before SourceTimeout, so default recovery does not fire for those callers. On main it fired for followers still waiting at SourceTimeout. This matches go/v0.27.0 and the approved leader-deadline bound.

Not re-run here: Replay, Mutations and integration lanes, and the description's benchmarks and scenario tables. We take those results as reported.

— Levicus Connecto 🤖

Comment thread go/engine.go Outdated
x.ctx = shared
go func() {
f.value, f.err = callSafely(func() (T, error) { return x.work(owner) })
f.expired = shared.Err() == context.DeadlineExceeded

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agent: F1 (medium, V). A follower in the same request can skip the retry when the nested timers fire in the wrong order.

With RequestLocal and coalescing on, the request flight and the process flight that it leads each get their own timer for the same deadline (line 242 runs once per level). If the process timer fires first, the process work ends with DeadlineExceeded. The request-flight goroutine waits on its own shared ctx, whose timer has not fired yet. So it takes f.done, gets that error, and this line records expired = false for the request flight. A second caller in the same request, with a later deadline, then skips the retry at line 230 and fails with the leader's DeadlineExceeded. On main that caller got the value.

Repro (fails 5 of 5 runs with go test -race -count=5):

func TestSameRequestFollowerRetriesAfterLeaderDeadline(t *testing.T) {
	for i := 0; i < 100; i++ {
		synctest.Test(t, func(t *testing.T) {
			f := newLifetimeFixture(Policy{RequestLocal: true})
			ctx, done := f.cache.Enable(context.Background())
			defer done()
			short, cancel := context.WithTimeout(ctx, time.Millisecond)
			defer cancel()
			load := f.firstHangs()
			leader := make(chan error, 1)
			go func() { _, err := f.get(short, load); leader <- err }()
			synctest.Wait()
			long, cancelLong := context.WithTimeout(ctx, time.Second)
			defer cancelLong()
			v, err := f.get(long, load)
			<-leader
			if err != nil || v != 7 {
				t.Fatalf("later-deadline follower = (%d, %v), want (7, nil)", v, err)
			}
		})
	}
}

On the real clock (5 ms leader, 1 s follower, 200 trials, no synctest) it failed 3 of 200.

Suggested fix: decide expiry from the leader's deadline, which both levels share:

deadline, hasDeadline := ctx.Deadline()
if hasDeadline {
	shared, cancel = context.WithDeadline(context.WithoutCancel(ctx), deadline)
} else {
	shared, cancel = context.WithCancel(context.WithoutCancel(ctx))
}
// ...
f.expired = hasDeadline && !time.Now().Before(deadline)

With this change the repro passes with -race -count=3, the 8 new tests pass (TestLoaderDeadlineErrorIsNotWorkExpiry included), and the real-clock run failed 0 of 200. Please keep the repro as a regression test: TestRequestFlightRetriesProcessWorkThatExpiredAtAnotherLeaderDeadline uses two requests, so it does not reach this path.

— Levicus Connecto 🤖

ctx, done := f.cache.Enable(context.Background())
defer done()

short, cancel := context.WithTimeout(ctx, 20*time.Millisecond)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agent: F2 (medium, V). This test does not prove that cancellation passes from the request flight to the process flight.

The caller leaves by a 20 ms deadline. The nested process flight inherits that deadline, so its work ends at 20 ms even if the request flight's last waiter did not cancel it. The code at the head is correct: the cancel-only test below passes. But a mutant where request flights get a no-op cancel (process flights unchanged) passes all 8 new tests. Only the cancel-only test catches it: live loaders = 1, published process flights = 1; want 0 and 0.

Suggested test:

func TestAbandonedRequestFlightCancelsProcessFlightWithoutDeadline(t *testing.T) {
	synctest.Test(t, func(t *testing.T) {
		f := newLifetimeFixture(Policy{RequestLocal: true})
		defer close(f.release)
		ctx, done := f.cache.Enable(context.Background())
		defer done()
		c, cancel := context.WithCancel(ctx)
		errc := make(chan error, 1)
		go func() { _, err := f.get(c, f.hang); errc <- err }()
		synctest.Wait()
		cancel()
		if err := <-errc; !errors.Is(err, context.Canceled) {
			t.Fatalf("caller error = %v, want its cancellation", err)
		}
		synctest.Wait()
		if live, leaders := f.live.Load(), f.cache.GetCoalescingState().Process.ActiveLeaders; live != 0 || leaders != 0 {
			t.Fatalf("live loaders = %d, published process flights = %d; want 0 and 0", live, leaders)
		}
	})
}

Optional: a variant where a second request keeps the process flight alive, to show that one request leaving does not end work that another request still waits for.

— Levicus Connecto 🤖

@lan17 lan17 left a comment •

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agent: Rollout-safety review of head 24fe78d (report mode, comment only).

Verdict: no blocker. Merging is safe for production after the required mutations run (G1). I found two low issues. F1 has a two-line fix that also covers F1 of the other review.

Why it is safe

  • Coalescing off does not change. singleFlight runs only when Coalesce is true (go/engine.go:154, :323, :329).
  • The waiter count changes only under c.mu. At zero, leave deletes the flight only if the map still holds it (go/engine.go:278-287). So it cannot remove a newer flight, or a request flight that the Enable close already cleared (go/cache.go:121).
  • Shadow jobs do not use the shared cancel. runShadow starts from context.WithoutCancel(x.ctx) (go/shadow.go:141), so cancel() after the work ends does not stop a shadow job. Remote reads also start from WithoutCancel and keep their own read budget (go/engine.go:491).
  • Release users move from go/v0.27.0, not from main: #255 is not in a release yet. On v0.27.0 a follower got its leader's error. Now it retries with its own time.
  • Every go/engine.go before anchor in mutations.json matches the head exactly once. PR CI is green on 24fe78d.

Findings

ID Severity Tag Where Issue Fix
F1 low V go/engine.go:252 expired is read after the work settles. A source timeout or source error that settled before the leader's deadline counts as expiry when the deadline passes before line 252 (a slow observer, codec or recovery predicate). Followers then start a second source call. C24 says they inherit the outcome. Keep deadline, hasDeadline := ctx.Deadline() and set f.expired = errors.Is(f.err, context.DeadlineExceeded) && hasDeadline && !time.Now().Before(deadline). This one check also fixes F1 of the review just before this one (nested timers). Update the M20 before anchor.
F2 low V go/engine.go:409, :231 Stale recovery needs x.ctx.Err() == nil, and x.ctx now has the leader's deadline. A follower needs the leader's remaining time plus SourceTimeout to get a stale value, not only SourceTimeout. No code change asked (approved design). Add it to Trade-offs and pin it with a test.
G1 medium (merge gate) V formal/catalogs/mutations.json The PR edits the mutation catalog, so the Mutations tier must pass before the PR leaves draft (AGENTS.md). No formal-full.yaml run exists for this branch, and the PR is already out of draft. Run gh workflow run formal-full.yaml --ref fix/go-cancel-abandoned-flights -f mode=mutations on the final head before merge.

Evidence. Scratch tests in package dialcache with testing/synctest, go test -race, Go 1.27.1. I did not push them.

F1: SourceTimeout 100 ms, a loader that waits for its ctx, leader deadline 120 ms, follower deadline 1 s, and an observer that sleeps 30 ms on the fallback error event.

Commit Follower result Time Source calls
24fe78d (head) FallbackTimeoutError 260 ms 2
f2b4f86 (main) FallbackTimeoutError 130 ms 1
go/v0.27.0 FallbackTimeoutError 130 ms 1
head with only the deadline-time check from the other review FallbackTimeoutError 260 ms 2
head with the combined check FallbackTimeoutError 130 ms 1

Each fix alone leaves the other bug: with only errors.Is(...) && shared.Err() == ..., the other review's nested-timer repro still fails 3 of 3 runs. With the combined check, both repros and the 8 tests in go/flight_lifetime_test.go pass (-race -count=3), and native go test -race . passes.

F2: a retained frame (value 7, RemoteTTL 1 s, StaleOnErrorMaxAge 1 h, age 2 s), SourceTimeout 200 ms, a hung loader, leader deadline 50 ms, follower deadline 230 ms.

Commit Follower result Time Source calls
24fe78d (head) context deadline exceeded 230 ms 2
f2b4f86 (main) stale value 7 200 ms 1
go/v0.27.0 the leader's context deadline exceeded 50 ms 1

F2 is a change from main only. It is not a regression from the last release.

Rollout caveats (no change asked)

  • During a source hang, a hot key starts one new source call per leader deadline, as on v0.27.0. The PR's own table shows 71 calls in 8 s, against 1 on main. A loader that ignores its ctx can stack these calls on its backend. Say this in the release notes.
  • NewRedisClient sets ContextTimeoutEnabled (go/redis_adapter.go:52). go-redis v9.22.0 puts only the ctx deadline on socket I/O, not its cancellation (internal/pool/conn.go, deadline). So the leader's deadline ends a stalled SET, but the last waiter's cancel does not stop a SET that was already sent. A plain go-redis client without ContextTimeoutEnabled uses neither; its own ReadTimeout and WriteTimeout bound the SET. After an unpublish, a new flight can overlap that old SET for the same key, as with coalescing off. The PR text already says "on a client that honors its context", so this is a note only.
  • Dark reads (remote ramped down) run the caller's source on the shared ctx. When the flight is cancelled, that shadow job reports source_error (go/shadow.go:237-240), the same as with coalescing off.

Inspected: the PR description, AGENTS.md, the full diff, go/engine.go, go/cache.go, go/shadow.go, go/deadline.go, go/redis_adapter.go, go-redis v9.22.0 (deadline, baseClient.context), singleFlight at go/v0.27.0, CONTRACTS rows C24 to C26, and the check runs and workflow runs on 24fe78d. go vet ./... passes. Native go test -race ./... passes at the head. I did not run the replay tests locally (they need Node); the go check passed in CI.

— Levicus Connecto 🤖

Comment thread go/engine.go Outdated
x.ctx = shared
go func() {
f.value, f.err = callSafely(func() (T, error) { return x.work(owner) })
f.expired = shared.Err() == context.DeadlineExceeded

@lan17 lan17 Nov 12, 2026 •

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agent: F1 (low, V). This reads shared.Err() after the work settles, and observers, the codec and the recovery predicate run before this line. If the source timed out or failed before the leader's deadline, and the deadline passes before this line, the flight is marked expired. Followers then retry and start a second source call, but C24 says they inherit the outcome.

Measured with testing/synctest: SourceTimeout 100 ms, leader deadline 120 ms, follower deadline 1 s, observer sleeps 30 ms on the fallback event. Head: 2 source calls, follower error at 260 ms. main and go/v0.27.0: 1 call, 130 ms.

The other review's F1 on this line (nested timers) needs the deadline-time check. That check alone still fails this case (2 calls, 260 ms), and errors.Is alone still fails its repro. One combined check fixes both:

deadline, hasDeadline := ctx.Deadline()
if hasDeadline {
	shared, cancel = context.WithDeadline(context.WithoutCancel(ctx), deadline)
} else {
	shared, cancel = context.WithCancel(context.WithoutCancel(ctx))
}
// ...
f.expired = errors.Is(f.err, context.DeadlineExceeded) && hasDeadline && !time.Now().Before(deadline)

With it, both repros and the 8 tests in go/flight_lifetime_test.go pass (-race -count=3). When the work ends because of the shared deadline, it returns plain context.DeadlineExceeded (awaitDeadline, readServing), so the errors.Is part still catches real expiry. The M20 before anchor in formal/catalogs/mutations.json contains these lines, so update it too.

— Levicus Connecto 🤖

Comment thread go/engine.go Outdated
// The work ran out of its leader's deadline. A follower with time left
// starts or joins fresh work instead of taking that deadline as its own.
if f.err != nil && f.expired {
return x.singleFlight(flights, owner, label)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agent: F2 (low, V). Stale recovery runs only when x.ctx.Err() == nil (go/engine.go:409), and x.ctx is now the shared ctx with the leader's deadline. So a follower gets a stale value only if its time covers the leader's remaining time plus a new SourceTimeout from this retry.

Measured: retained frame, SourceTimeout 200 ms, hung loader, leader deadline 50 ms, follower deadline 230 ms. Head: context deadline exceeded at 230 ms, 2 source calls. main: stale value at 200 ms. go/v0.27.0: the leader's error at 50 ms. So this is no regression from the last release. I ask for no code change, because the leader-deadline bound is the approved design. Please add it to Trade-offs and pin it with a test, so a later change does not move it by accident.

— Levicus Connecto 🤖

Cancelling a flight and removing it from the map let the next caller start the source again while a source that ignores its context was still running, once per caller deadline. A cancelled flight now stays published until its work returns, and shared work waits for its source until the budget instead of its own context; callers still leave on their own contexts. A follower whose shared work failed after the work's context ended starts or joins fresh work.

Work that no caller waits for any more fills no cache.
Shared work now waits for a source that ignores its context, so it could return a late value after every caller had left. Only the remote fill skipped it: the local layer, the request memo and a coalesced dark-shadow fill still wrote it, and the dark-shadow write could land above an invalidation watermark.

Every fill now asks one question: does a caller still wait for this work? For a flight that is whether it has a waiter; outside a flight, whether the caller's context is live. A dark-shadow fill of uncoalesced work still outlives its caller.
Ending shared work at the leader's deadline threw away work that live followers still needed: a source slower than the callers' deadlines never filled the cache, and followers lost stale recovery. It also departed from the documented rule that a caller timeout does not by itself cancel shared work. Shared work now ends only when no caller waits for it, or at SourceTimeout.

A follower retries only when it joined work that was cancelled because no caller waited for it.
@lan17 lan17 changed the title fix(go): end shared work when its last waiter leaves or its leader's deadline passes fix(go): cancel shared work that no caller waits for Nov 12, 2026
A follower that joined cancelled work retries with its own context. The work unpublishes its flight before it settles, so a retry always finds fresh work or none. If that invariant broke, the follower would rejoin the same settled flight forever; it now takes that flight's result instead.

@lan17 lan17 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agent: Round 2 review of the delta 24fe78d..c6f9136 (head c6f9136) in report mode. Lanes: correctness, concurrency and tests. This is a comment review. It does not approve and does not request changes.

Result: 1 finding (medium). No blocker and no high. All three lanes found it on their own. The rest of the delta does what the description says: one source per key while it runs, a cancelled flight stays published until its work returns, a follower retries only after cancelled work has left the registry, and abandoned work skips the local, request memo and coalesced shadow fills. The lifetime tests and ./internal/testsuite/... pass at the head with -race on Go 1.27.1.

ID Severity Tag Location Fix
R2-F1 medium V go/engine.go:445 Pass func() bool { return !x.abandoned() } as the allowed argument of this putRemote, so the check runs after the encode and right before the write. Keep a blocked-encoder test.

Tag V: we ran it or read the code at the head. R2-F1 has an inline comment with a repro and the fix.

Round 1 findings at this head:

  • F1 (nested timers and expiry): no longer applies, because the leader deadline is gone.
  • F2 (nested cancel test): fixed by TestCancelledRequestFlightCancelsTheProcessFlightItWaitsFor. A mutant where request flights get a no-op cancel now fails this test and TestAbandonedRequestFlightCancelsTheProcessFlightItWaitsFor.
  • F3 (mixed-deadline cost): no longer applies.

Question (not a finding): with NoTimeout and a source that ignores its context, a cancelled flight waits for the source with no bound and stays published. So the key stays held until the source returns. This is the same as main and follows from "a source that ignores its context is not started again". One line in the description or docs could say it.

Not re-run here: Replay, Mutations and integration lanes, the formal-full run in progress, and the benchmark and scenario tables. We take those as reported.

— Levicus Connecto 🤖

Comment thread go/engine.go Outdated
return v, nil
}
if remote.kind != "error" {
if _, writeErr := x.putRemote(v, remote.fence, "remote", nil); writeErr != nil {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agent: R2-F1 (medium, V). Work can still fill Redis after its last waiter left, when the waiter leaves during the encode.

The abandoned() check at line 441 runs before putRemote encodes and compresses the value. This call passes nil for allowed, so putRemote does not check again before the write. If the last waiter leaves while the codec runs, the encode finishes and Remote.Write is called for work that nobody waits for. The source returned before the cancel, and the encode and the write both succeed, so this is not the follow-up about errors inside an encode or a write.

Repro (fails at c6f9136 with go test -race -count=1: writes started after last waiter left = 1, want 0):

func TestCancellationDuringEncodeStartsNoWrite(t *testing.T) {
	synctest.Test(t, func(t *testing.T) {
		var writes atomic.Int64
		release := make(chan struct{})
		remote := boundaryRemote{
			read: func(context.Context) (ReadResult, error) {
				return ReadResult{Kind: "miss", Reason: "value_absent"}, nil
			},
			write: func(Frame) error { writes.Add(1); return nil },
		}
		cache := MustNew(WithRemote(remote), WithoutCompression())
		op := Operation[any]{
			Identity: Identity{Namespace: "lifetime", KeyType: "id", ID: "encode", UseCase: "get"},
			Policy:   Policy{RemoteTTL: time.Minute},
			Codec: boundaryCodec{encode: func(any) (Payload, error) {
				<-release
				return Payload{Bytes: []byte("7")}, nil
			}},
		}
		ctx, done := cache.Enable(context.Background())
		defer done()
		caller, cancel := context.WithCancel(ctx)
		result := make(chan error, 1)
		go func() {
			_, err := GetOrLoad(caller, cache, op, func(context.Context) (any, error) { return 7, nil })
			result <- err
		}()
		synctest.Wait()
		cancel()
		if err := <-result; !errors.Is(err, context.Canceled) {
			t.Fatalf("caller error = %v, want its cancellation", err)
		}
		close(release)
		synctest.Wait()
		if got := writes.Load(); got != 0 {
			t.Fatalf("writes started after last waiter left = %d, want 0", got)
		}
	})
}

The impact depends on the adapter. x.ctx is already cancelled at the write. go-redis v9.22.0 checks the context before it takes a connection, so with RedisAdapter we expect a rejected write plus a spurious cache_write error and warning, not a stale value. A custom remote that does not check its context stores the abandoned value, and that write can land after an invalidation. That is the case this PR fixes.

Suggested fix, the same gate that the shadow fill uses:

if _, writeErr := x.putRemote(v, remote.fence, "remote", func() bool { return !x.abandoned() }); writeErr != nil {

With this change the repro and two other variants of it pass with -race -count=3, and the lifetime, shadow and coalescing tests and ./internal/testsuite/... still pass. The check at line 441 can stay, because it skips the encode for work that was already abandoned. A write that has already started still depends on the adapter honoring its context.

— Levicus Connecto 🤖

…ding

The no-fill check ran before the value was encoded, so a caller that left while the codec ran still let the write through. The check now also runs right before the write.

@lan17 lan17 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agent: Final rollout-safety review of head 2d2c3f7 (report mode, comment only). It replaces my review of 24fe78d.

Verdict: no blocker. Merging is safe for production. I found one medium issue and one low issue, both in the source-timeout path of cancelled work. Both have small fixes.

My earlier findings

  • F1 (expiry read after the work settles): gone, because there is no leader deadline now. My repro gives 1 source call at 130 ms, the same as main.
  • F2 (stale recovery with mixed deadlines): fixed. My repro serves the stale value at 200 ms, the same as main.
  • G1 (mutations run): done. formal-full.yaml in mutations mode passed on 2d2c3f7 (run 43167821481), and every PR check passes.

Why it is safe

  • Coalescing off keeps its source wait on the caller's context (go/engine.go:500). Only its fills change: they now need the caller's context to be live (waited, go/engine.go:297-302). That matters only between the source result and the fill.
  • leave only cancels (go/engine.go:288-293). The work unpublishes its own flight before it settles (go/engine.go:264-267), and a follower retries only after that (go/engine.go:241-248). So a settled flight that stays in the registry gives a result, not a spin.
  • Work with no waiter fills no cache: remote (go/engine.go:441, :446), local (go/engine.go:463), request memo (go/engine.go:189), coalesced dark-shadow fill (go/shadow.go:249). abandoned takes c.mu, and none of its callers holds c.mu.
  • Shadow jobs and remote reads still start from WithoutCancel (go/shadow.go:141, go/engine.go:530). The shared ctx has no deadline now, so the Redis client's own timeouts bound a write, as on main (250 ms with NewRedisClient; ReadTimeout and WriteTimeout for a plain go-redis client).
  • Every Go before anchor in mutations.json matches the head exactly once.

Findings

ID Severity Tag Where Issue Fix
N1 medium V go/engine.go:434, :514 (cause :505) A caller that joins cancelled work that is still running gets no stale recovery and no fallback event. After cancellation, shared work still waits for its source (:505), so the source can time out while the new caller waits. But recovery (:434) and the failure event (:514) still test x.ctx.Err() == nil, and x.ctx is the cancelled shared ctx. The caller then retries with a new full SourceTimeout and fails at its own deadline. Use !x.abandoned() instead of x.ctx.Err() == nil at :434 and :514.
N2 low V go/engine.go:261 A source timeout that settled in cancelled work counts as cut short. The caller that joined retries with a new budget, but C24 says it inherits the timeout. f.cutShort = shared.Err() != nil && !errors.As(f.err, new(*FallbackTimeoutError)). Update the M20 before anchor, which contains this line.

Evidence. I used scratch tests in package dialcache with testing/synctest, go test -race and Go 1.27.1. I did not push them. Setup: a retained frame (value 7), SourceTimeout 100 ms, and a source that ignores its ctx and hangs. Caller A (30 ms deadline) starts the work and leaves. Caller B arrives at 40 ms with a 150 ms deadline and joins the cancelled work.

Commit B, stale-on-error on B, stale-on-error off Source calls (on / off) fallback events
head 2d2c3f7 context deadline exceeded, 190 ms context deadline exceeded, 190 ms 2 / 2 0
main f2b4f86 stale value 7, 100 ms FallbackTimeoutError, 100 ms 1 / 1 1
go/v0.27.0 stale value 7 FallbackTimeoutError 2 / 2 2
head with the N1 fix stale value 7, 100 ms context deadline exceeded, 190 ms 1 / 2 1
head with the N1 and N2 fixes stale value 7, 100 ms FallbackTimeoutError, 100 ms 1 / 1 1

On v0.27.0, A waits past its own deadline, so B starts later and the times do not compare. With both fixes, the 13 tests in go/flight_lifetime_test.go pass with -race -count=3, ./internal/testsuite passes, and the native go test -race . tests pass.

N1 occurs during an outage. A source that ignores its ctx hangs. The key gets calls with gaps longer than the caller deadline, so each caller joins work that is already cancelled. This is the case that stale-on-error is for, and B gets an error where main and v0.27.0 serve the stale value.

Rollout caveats (no change asked)

  • With NoTimeout, a source that ignores its ctx and never returns keeps its flight until restart. Later callers join it and fail at their own deadline. In my test there was still 1 source call after 10 minutes. main does the same; on v0.27.0 the caller itself hangs. Add one line to the release notes, next to the hot-key note.
  • A cold key whose source honors its ctx but is slower than every caller's deadline never fills. Each flight is cancelled before its source returns (as on v0.27.0; main filled it in the background). The description says that abandoned work fills no cache; the release notes can name this effect.

Inspected: the PR description, the diff from main and from 24fe78d, go/engine.go, go/cache.go, go/shadow.go, go/deadline.go, the 13 new tests and the testsuite additions, and the check runs and formal-full run on 2d2c3f7.

— Levicus Connecto 🤖

Comment thread go/engine.go
// Waiters leave on their own contexts, so shared work waits for its source
// until the budget. A source that ignores a cancelled context then still
// holds its flight, and the key gets no second copy of it.
wait = context.Background()

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agent: N1 (medium, V). After the last waiter leaves, this wait still runs until the source returns or SourceTimeout. A new caller can join the flight in that time. When the source times out, two checks still use x.ctx.Err() == nil, and x.ctx is the cancelled shared ctx:

  • go/engine.go:434: stale recovery is skipped.
  • go/engine.go:514: the fallback failure event is skipped.

So the caller that joined gets no stale value. It retries with a new full SourceTimeout, and fails at its own deadline.

Measured: retained frame, SourceTimeout 100 ms, a source that ignores its ctx. A (30 ms deadline) leaves. B joins at 40 ms with a 150 ms deadline. Head: B gets context deadline exceeded at 190 ms, with 2 source calls and 0 fallback events. main: B gets the stale value at 100 ms. go/v0.27.0: B gets the stale value.

Fix: at both lines, use the waiter test that the fills already use:

if remote.kind != "error" && remote.kind != "decode_error" && p.StaleOnErrorMaxAge > 0 && !x.abandoned() && x.canRecover(err) {
if err != nil && !x.abandoned() {

Outside a flight, abandoned() is x.ctx.Err() != nil, so coalescing off does not change. With this fix, B gets the stale value at 100 ms with 1 source call. The 13 tests in go/flight_lifetime_test.go and ./internal/testsuite still pass (-race -count=3). Neither line is in a mutation anchor.

— Levicus Connecto 🤖

Comment thread go/engine.go Outdated
x.flight = f
go func() {
f.value, f.err = callSafely(func() (T, error) { return x.work(owner) })
f.cutShort = shared.Err() != nil

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agent: N2 (low, V). After cancellation, the source wait ignores the cancel, so this work can settle with a real FallbackTimeoutError. This line still marks that result as cut short. A caller that joined the cancelled work then retries with a new budget. C24 says it inherits the remaining source time and the outcome.

Measured (same setup as N1, with stale-on-error off): head gives B context deadline exceeded at 190 ms with 2 source calls. main gives FallbackTimeoutError at 100 ms with 1 call. With this change and the N1 fix: FallbackTimeoutError at 100 ms, 1 call. All 13 new tests and ./internal/testsuite still pass. The test does not use errors.Is(f.err, context.Canceled), because a loader can map cancellation to its own error type (a gRPC status, for example), and then a cancelled follower would not retry.

Suggested change
f.cutShort = shared.Err() != nil
f.cutShort = shared.Err() != nil && !errors.As(f.err, new(*FallbackTimeoutError))

The M20 before anchor in formal/catalogs/mutations.json contains this line, so update it too.

— Levicus Connecto 🤖

@lan17 lan17 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agent: Final review of the whole PR (f2b4f86..2d2c3f7, head 2d2c3f7) in report mode. Lanes: correctness, concurrency, tests, reliability, performance, simplicity and holistic. The holistic lane made an independent pass first, then audited the findings. This is a comment review. It does not approve and does not request changes.

Result: 1 finding (low). No blocker, no high and no medium. The correctness, tests, reliability, performance and simplicity lanes found no issue. The PR does what the description says: the last waiter cancels the shared work, and the cancellation passes through request and process flights; a source runs once per key while it runs; a follower retries only after cancelled work has left the registry; abandoned work fills no remote, local, request memo or coalesced shadow cache; and a caller that already left starts no work. The 13 lifetime tests and ./internal/testsuite/... pass at the head with -race on Go 1.27.1.

ID Severity Tag Location Fix
R3-F1 low V go/engine.go:680 Move the allowed check in putRemote to just before Write, after writeTimestamp and the TTL clamp event. Check the effects replay for the moved events.

Tag V: we ran it or read the code at the head. The inline comment for R3-F1 is on line 446, because line 680 is not in the diff.

Earlier rounds at this head:

  • R2-F1 (a fill after the last waiter leaves during the encode) is fixed. TestCallerLeavingDuringEncodeStopsTheWrite fails at c6f9136 (1 write) and passes at 2d2c3f7. Our three repros from round 2 also pass.
  • Round 1: F2 stays fixed; F1 and F3 no longer apply.

Questions (not findings):

  • A flight that completes keeps waiters > 0, so a deferred coalesced dark-shadow fill stays allowed after every caller has returned. This matches the stated rule: only work cancelled for lack of waiters is blocked. If that is the intent, nothing to do.
  • With NoTimeout and a source that ignores its context, a cancelled flight stays published with no bound. This is the same as main (raised in round 2).

Not re-run here: the replay and effects conformance tests (Node is not installed in our sandbox), Mutations, integration, and the benchmark and scenario tables. We take the description's results as reported, including the green formal-full run at 2d2c3f7.

— Levicus Connecto 🤖

Comment thread go/engine.go
if remote.kind != "error" {
if _, writeErr := x.putRemote(v, remote.fence, "remote", nil); writeErr != nil {
// The last caller can also leave while the value is being encoded.
if _, writeErr := x.putRemote(v, remote.fence, "remote", func() bool { return !x.abandoned() }); writeErr != nil {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agent: R3-F1 (low, V). The allowed hook still runs before two callbacks, so a write can start after the last waiter left.

In putRemote, the allowed check (line 680) runs before writeTimestamp (line 683, which calls the caller's clock) and before the tracked_ttl_clamped event (lines 694 to 696, which calls the observers). If one of these blocks and the last waiter leaves during it, Write (line 699) still runs. The cancel lands before the write starts, so this is not the documented case of a write already in progress.

Repro (fails 3 of 3 at 2d2c3f7 with -race: writes begun after the last waiter left = 1, want 0):

func TestCallerLeavingDuringClampObserverStopsTheWrite(t *testing.T) {
	synctest.Test(t, func(t *testing.T) {
		entered, release := make(chan struct{}), make(chan struct{})
		var writes atomic.Int64
		remote := boundaryRemote{
			read: func(context.Context) (ReadResult, error) {
				return ReadResult{Kind: "miss", Reason: "value_absent"}, nil
			},
			write: func(Frame) error { writes.Add(1); return nil },
		}
		cache := MustNew(WithRemote(remote), WithoutCompression(), WithObserver(func(e Event) {
			if e.Kind == "error" && e.Data["error"] == "tracked_ttl_clamped" {
				close(entered)
				<-release
			}
		}))
		ctx, done := cache.Enable(context.Background())
		defer done()
		ctx, cancel := context.WithCancel(ctx)
		defer cancel()
		op := Operation[int]{
			Identity: Identity{Namespace: "lifetime", KeyType: "id", ID: "fill", UseCase: "get", Tracked: true},
			Policy:   Policy{RemoteTTL: 2 * time.Hour},
		}
		result := make(chan error, 1)
		go func() {
			_, err := GetOrLoad(ctx, cache, op, func(context.Context) (int, error) { return 7, nil })
			result <- err
		}()
		<-entered
		cancel()
		if err := <-result; !errors.Is(err, context.Canceled) {
			t.Fatalf("caller error = %v, want its cancellation", err)
		}
		close(release)
		synctest.Wait()
		if n := writes.Load(); n != 0 {
			t.Fatalf("writes begun after the last waiter left = %d, want 0", n)
		}
	})
}

The impact is small. The trigger needs a blocking callback in that gap: an observer on tracked_ttl_clamped (only for tracked keys whose remote TTL or stale-on-error age is above 1 h) or a blocking clock. On this serving path x.ctx is already cancelled, so go-redis rejects the write. The coalesced dark-shadow fill (go/shadow.go:248) uses the same check, but its write runs on the detached job context, so there the check is the only guard.

Suggested fix: move the check to just before Write:

	if x.op.Identity.Tracked && ttl > time.Hour {
		ttl = time.Hour
		x.errorEvent(layer, "tracked_ttl_clamped", false)
	}
	if allowed != nil && !allowed() {
		return false, nil
	}
	_, err = callSafely(func() (struct{}, error) {

With this change the repro passes with -race -count=3. The 13 lifetime tests, the shadow and coalescing tests, the encode repros and ./internal/testsuite/... still pass. We could not run the effects replay here. After the move, a fill that the check drops still reads the clock and can emit tracked_ttl_clamped, so please check the replay before you take it. If replayed events must not change, keep the current check and add a second one just before Write.

— Levicus Connecto 🤖

A caller can join a cancelled flight while a source that ignores its context still runs. Stale recovery and the fallback event looked at the flight's context, so that caller got neither and started the source again. Both now ask whether a caller still waits, like the cache fills. A source timeout is the work's own outcome, so a joined caller inherits it instead of retrying.

The no-fill check in a remote write now runs right before the write, after the writer's clock and the tracked TTL event.
errors.As calls the source error's own methods. It ran after callSafely
returned, so a typed-nil error whose Unwrap dereferences its receiver
crashed the process when the only caller had left. The classification now
runs inside the recovered closure. The no-fill check also runs before the
tracked TTL clamp, so a skipped write emits no clamp event.
The tracked TTL clamp event runs observers synchronously. With the no-fill
check before it, a slow observer let the last caller leave and the
abandoned work still wrote to Redis. Keep the early check, so a skipped
write emits no clamp event, and check again right before the write.
A caller that arrives after explicit invalidation can join a flight that
read Redis before it, even when every earlier caller has stopped waiting:
the flight stays open until its source settles or times out.

@lan17 lan17 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agent: Rollout-safety review of the delta 26cdf05..97f52ad (3 commits; report mode, comment only). Earlier rounds covered the rest of the PR.

Verdict: no blocker. The delta is safe to merge and to roll out. I found no code defect. One CI gate is open: the required test check is red on 97f52ad, because the Codecov uploader failed in the rust job.

Why it is safe

  • P6 (217dd66): the flight goroutine now runs errors.As inside callSafely (go/engine.go:260-267), so a source error whose methods panic gives CallbackPanicError and does not end the process. After callSafely, the goroutine runs no user code (go/engine.go:268-274). errors.As runs only when the shared ctx was cancelled, so live flights still return the source error unchanged. f.cutShort is still written before close(f.done), so followers read it safely.
  • P7 (22a9daa): putRemote now checks allowed() before the clamp event (go/engine.go:694) and again right before Remote.Write (go/engine.go:707). No user code runs between the second check and the write. The stamp is still taken before the checks (go/engine.go:686). So a slow clamp observer can only make a frame look older than its write. For invalidation this is the safe direction: an older stamp is fenced, not served. The coalesced dark-shadow fill uses the same allowed closure (go/shadow.go:248-250), so it gets both checks too.
  • D3 docs (97f52ad): the new sentence in docs/coalescing.md:89-91 matches the code: a cancelled flight stays in the registry until its work returns. It also matches the other ports, which run flight work on a spawned task or a shielded future. docs/invalidation.md already tells users to include "remaining fallback work" in Dmax when they choose futureBufferMs.
  • Every Go before anchor in mutations.json matches the head exactly once (82 anchors), M20 included.

Verified locally (Go 1.27.1, -race)

  • Head 97f52ad: the lifetime, follower, source and panic tests in go/flight_lifetime_test.go pass with -count=3, and ./internal/testsuite passes with -count=3. The native go test -race . tests pass, and go vet ./... passes. I did not run the replay tests (they need Node).
  • With go/engine.go from 26cdf05: TestPanickingErrorFromAbandonedWorkDoesNotCrash ends the test binary with a nil pointer dereference.
  • With go/engine.go from 217dd66: TestCallerLeavingDuringTheTTLClampEventStopsTheWrite fails with "writes = 1, want 0".

Findings

ID Severity Tag Where Issue Fix
G1 gate (CI) V CI run 43324223350, job rust The Rust tests passed, but the Codecov upload step failed: "gpg: no valid OpenPGP data found", then "Could not verify signature". So rust and the required test check are red on 97f52ad. The delta changes no Rust code. Re-run the failed jobs: gh run rerun 43324223350 --failed.
I1 info V go/engine.go:265 When the work of a cancelled flight panics, f.cutShort stays false. So a caller that joined after the cancel gets CallbackPanicError and does not retry. At 26cdf05 it retried. A follower of a live flight gets the same error, so this is consistent. No change asked.

Outside this delta (no change asked): a coalesced dark-shadow fill that is skipped because no caller waits reports the shadow outcome fill_fenced (go/shadow.go:261). Nothing fenced it. If that outcome feeds a dashboard, a later change can give it its own name.

— Levicus Connecto 🤖

Comment thread go/engine.go Outdated
// A source timeout is the work's own outcome, which a follower inherits.
// errors.As runs the source error's own methods, so a panic in them is
// recovered here like any other source panic.
f.cutShort = shared.Err() != nil && !errors.As(err, new(*FallbackTimeoutError))

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agent: I1 (info, V). The assignment is now inside the callSafely closure. So when the work itself panics, or errors.As panics, f.cutShort stays false. A caller that joined the cancelled flight then gets CallbackPanicError and does not retry. At 26cdf05, cutShort came from f.err after callSafely, so that caller retried. A follower of a live flight gets the same CallbackPanicError, so this is consistent, and the main point of P6 holds: the process no longer crashes. No change asked.

— Levicus Connecto 🤖

@lan17 lan17 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agent: Final delta review of 26cdf05..97f52ad (head 97f52ad) in report mode. Lanes: correctness, concurrency, tests, reliability, simplicity, contracts and holistic. The holistic lane made an independent pass first, then audited the findings. Performance, security and architecture do not apply to this delta. This is a comment review. It does not approve and does not request changes.

Verdict: 1 medium finding to fix before the formal mutations run, and 1 low test gap. No blocker and no high. The fix for R4-F1 changes go/engine.go, so a run on 97f52ad would not cover the final head.

ID Severity Tag Location Fix
R4-F1 medium V go/engine.go:265 Keep x.work under its own callSafely, and run the errors.As classification after it in a second callSafely. A panic in either one then leaves cancelled work retryable.
R4-F2 low V go/engine.go:693 Add a test that a write skipped before the clamp block emits no tracked_ttl_clamped event.

Tag V: we ran it or read the code at the head. Both findings have inline comments.

What is right in this delta:

  • The flight goroutine no longer crashes the process when errors.As panics. TestPanickingErrorFromAbandonedWorkDoesNotCrash panics at 26cdf05 and passes at the head.
  • The second allowed() check closes the observer window. TestCallerLeavingDuringTheTTLClampEventStopsTheWrite fails at 217dd66 (1 write) and passes at the head. Deleting the second check fails it again.
  • The docs/coalescing.md sentence matches the code: a cancelled flight stays published until its work returns or reaches SourceTimeout.

Question (not a finding, outside this delta): on the uncoalesced path, the default canRecover predicate (go/engine.go:736) calls errors.As on the source error in the caller's goroutine, with no recover. The same panicking error then panics in the caller. This was the case before this PR.

Not re-run here: the replay and effects conformance tests (Node is not installed in our sandbox), Mutations and integration. We take the description's local results as reported.

— Levicus Connecto 🤖

Comment thread go/engine.go Outdated
// A source timeout is the work's own outcome, which a follower inherits.
// errors.As runs the source error's own methods, so a panic in them is
// recovered here like any other source panic.
f.cutShort = shared.Err() != nil && !errors.As(err, new(*FallbackTimeoutError))

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agent: R4-F1 (medium, V). A panic inside x.work now skips this line, so a late follower of cancelled work gets CallbackPanicError instead of retrying.

f.cutShort is set inside the callSafely closure, after x.work. If x.work panics, callSafely recovers it, but this line never runs and cutShort stays false. A follower that joined the cancelled flight then fails the check at line 241 and takes the error. At 26cdf05, x.work was recovered first and this classification ran after it, so cutShort was true and the follower retried.

One reachable path uses this PR's own error type, with stale recovery on and the default predicate:

  1. The only caller leaves while the loader is blocked.
  2. A new caller joins the still-published flight.
  3. The loader returns a typed-nil *nilUnwrapError.
  4. The new waiter makes abandoned() false, so traverse calls canRecover (line 440). Its errors.As (line 736) panics inside x.work.

A panicking caller clock inside the work takes the same path.

Repro (fails 3 of 3 at 97f52ad with -race: follower=(0, *dialcache.CallbackPanicError), calls=1; want (7, nil), calls=2; passes at 26cdf05):

func TestFollowerOfCancelledWorkRetriesAfterAPanicInTheWork(t *testing.T) {
	synctest.Test(t, func(t *testing.T) {
		age := time.Hour
		f := newLifetimeFixture(Policy{StaleOnErrorMaxAge: &age})
		ctx, done := f.cache.Enable(context.Background())
		defer done()
		release := make(chan struct{})
		load := func(context.Context) (int, error) {
			if f.calls.Add(1) == 1 {
				<-release
				var err *nilUnwrapError
				return 0, err
			}
			return 7, nil
		}
		short, cancel := context.WithTimeout(ctx, time.Millisecond)
		defer cancel()
		_, _ = f.get(short, load)
		follower := make(chan error, 1)
		var v int
		go func() {
			var err error
			v, err = f.get(ctx, load)
			follower <- err
		}()
		synctest.Wait()
		close(release)
		if err := <-follower; err != nil || v != 7 || f.calls.Load() != 2 {
			t.Fatalf("follower = (%d, %T), calls = %d; want (7, nil), calls = 2", v, err, f.calls.Load())
		}
	})
}

Suggested fix: recover the work and the classification separately.

		f.value, f.err = callSafely(func() (T, error) { return x.work(owner) })
		if shared.Err() != nil {
			// A source timeout is the work's own outcome, which a follower inherits.
			// errors.As runs the error's own methods, so it is recovered too.
			timedOut, _ := callSafely(func() (bool, error) { return errors.As(f.err, new(*FallbackTimeoutError)), nil })
			f.cutShort = !timedOut
		}

With this change the repro, TestPanickingErrorFromAbandonedWorkDoesNotCrash and TestCallerLeavingDuringTheTTLClampEventStopsTheWrite pass with -race -count=3. The lifetime, shadow, coalescing, stale and timeout tests and ./internal/testsuite/... still pass with -race. It still uses errors.As, not x.timedOut, so nested flights stay apart. M20's anchors and the engine.go pins need a refresh.

Why medium and not high: the trigger needs a panic inside shared work, which is unusual. But this delta adds it, and it breaks the PR's rule that a follower of cancelled work starts fresh work.

— Levicus Connecto 🤖

Comment thread go/engine.go
if fence != nil && uint64(stamp) <= *fence {
return false, nil
}
// Checked before the clamp event too, so a skipped write emits none.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agent: R4-F2 (low, V). No test covers the reason for this first check.

The comment says this check means a skipped write emits no tracked_ttl_clamped event. A mutant that deletes this check (lines 693 to 696) still passes every lifetime, shadow, coalescing, clamp and encode test and ./internal/testsuite/..., because the second check still stops the write. Deleting the second check fails TestCallerLeavingDuringTheTTLClampEventStopsTheWrite, so that one is covered.

Suggested test: use a tracked key with a TTL above 1 h and have the only caller leave during the encode (as in TestCallerLeavingDuringEncodeStopsTheWrite). Then assert 0 writes and no tracked_ttl_clamped event.

— Levicus Connecto 🤖

Classifying the error inside the work's own recovered closure skipped the
classification when the work panicked, so a caller that joined cancelled
work got the panic instead of starting fresh work. The work and the
timeout check now each run under their own recover; an error whose methods
panic is not a timeout. The encode test also checks that a write skipped
before the tracked TTL clamp emits no clamp event.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants