fix(meta): keep reader temp info alive with a session lease

[tinswzy]

Summary

Reader temp info was created with a fixed 60s lease that nothing ever renewed — an etcd Put with WithLease re-binds the key but does not refresh the lease TTL — so every reader older than 60s silently lost its metadata, and every subsequent UpdateReaderTempInfo (a strict get-then-put) failed with reader temp info not found forever. Since this metadata is what protects truncated segments still needed by lagging readers from physical cleanup, the protection silently switched off for exactly the readers it exists for. See #265 for the full analysis; observed in the field as part of milvus-io/milvus#52341.

Key changes

• CreateReaderTempInfo now guards the key with a concurrency.Session (automatic keepalive at ~TTL/3, mirroring the existing writer-lock pattern). Liveness is decoupled from read progress, so idle/slow readers keep their metadata; a crashed process still auto-expires within the TTL. Reopening the same reader name swaps in a fresh session and closes the stale one.
• UpdateReaderTempInfo is now an unconditional upsert rebuilt from the reader's cached open position, gated on the in-process session entry:
  • an owned reader self-heals its key after lease loss (e.g. an etcd partition outliving the TTL), rebuilding the session and retrying once;
  • a reader without an in-process session (closed, or a key written by someone else) is rejected with the existing reader temp info not found error — blind adoption would resurrect a key with a live keepalive after close and pin the cleanup low-watermark forever.
• DeleteReaderTempInfo / provider Close() close the session first (revoking the lease deletes the key immediately), keeping the explicit etcd delete as a fallback for keys not owned by this process.
• The TTL is now the package variable readerTempInfoSessionTTLSeconds (default unchanged at 60) so tests can shrink it.

No key layout or value format changes; mixed-version deployments keep working (keys written by old clients keep the old 60s-expiry behavior).

Testing

Written test-first (both regressions verified failing on master before the fix):

• test reader temp info outlives lease ttl while idle — an idle reader's metadata survives 2.5× the lease TTL and stays attached to a live lease (fails on master: the key expires).
• test update reader temp info self heals after lease loss — after the lease is revoked and the key removed, the next update recreates the key, preserves the open position, and re-attaches a live lease (fails on master: reader temp info not found).
• test update after delete does not resurrect reader temp info — a late update after close is rejected and the key stays deleted (guards the deliberate non-resurrection gate).
• Existing testUpdateReaderTempInfoWithoutLease renamed to testUpdateReaderTempInfoWithoutSession with inverted semantics to pin the new gate.
• go test ./meta/ and go test ./woodpecker/log/ pass; golangci-lint run ./meta/... reports 0 issues.

Known boundaries (deliberately out of scope): the stuck-retry behavior on no record extract from milvus-io/milvus#52341 is a separate defect; "process crash → key expires within TTL" relies on etcd session semantics and is not separately unit-tested.

Fixes #265

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 73.10924% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.64%. Comparing base (d3167df) to head (c2874da).

Files with missing lines	Patch %	Lines
meta/metadata_etcd.go	73.10%	27 Missing and 5 partials ⚠️

❌ Your patch check has failed because the patch coverage (73.10%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #266      +/-   ##
==========================================
- Coverage   83.69%   83.64%   -0.06%     
==========================================
  Files         182      182              
  Lines       25458    25544      +86     
==========================================
+ Hits        21307    21366      +59     
- Misses       3172     3194      +22     
- Partials      979      984       +5     

Components	Coverage Δ
Server	83.36% <ø> (+0.05%)	⬆️
Client	90.47% <ø> (ø)
Meta	85.91% <73.10%> (-1.08%)	⬇️
Common	88.63% <ø> (-0.08%)	⬇️

Files with missing lines	Coverage Δ
meta/metadata_etcd.go	85.48% <73.10%> (-1.10%)	⬇️

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[czs007]

You've hit your session limit · resets 11:20am (America/Los_Angeles)

[czs007]

Re-review of 97c15df802df

No findings from this mechanism were published on this commit previously, so there is nothing from an earlier round to confirm or retract. Everything below is from this round.

---

This round's review found lifecycle problems in the new reader temp-info session handling in meta/metadata_etcd.go — failed writes and post-close updates can both leave a reader without metadata protection or leave an untracked session behind — plus one missing-timeout regression and one test-hygiene item.

High

• meta/metadata_etcd.go:1368 — any failed Put revokes the session, deleting the live reader's temp info key. The retry loop calls session.Close() unconditionally when e.client.Put returns an error, without distinguishing the error source. concurrency.Session.Close() is Orphan() + Revoke(leaseID), and revoking makes etcd delete every key attached to that lease — including this reader's own temp info key. The most common Put failures have nothing to do with lease health: ctx1 comes from e.getContextWithTimeout(ctx) (line 1358), so a cancelled caller context or an etcd slowdown past e.requestTimeout yields a context error while the lease is perfectly healthy. Both attempts derive from the same (already dead) parent context, so the second Put fails identically. Failure: a Put fails because the caller's context was cancelled or etcd exceeded e.requestTimeout while the lease is healthy → the healthy lease is revoked and the reader temp info key is deleted; the reader keeps reading with no metadata protection, so concurrent truncated-segment cleanup can remove segments it has not yet consumed, and the key is only rebuilt on the next successful update — potentially never for an idle or lagging reader. The caller at woodpecker/log/log_reader.go:160-163 only logs a warning and keeps reading. Suggestion: rotate the session only when session.Done() is already closed or etcd explicitly reports the lease is gone (rpctypes.ErrLeaseNotFound); return context errors and other unrelated write errors without touching the existing lease. (raised by nsj)

• meta/metadata_etcd.go:1301 — TOCTOU between Update's pre-lock Load and Delete's LoadAndDelete: a closed reader's key can be resurrected onto an untracked session. UpdateReaderTempInfo loads the entry pointer at line 1301 and only locks it at line 1309, while DeleteReaderTempInfo removes the entry from the map first (line 1402) and then locks it to close the session (lines 1404-1409). With the interleaving Update.Load → Delete completes fully (including the etcd delete at line 1415) → Update acquires the lock, the update sees a closed session.Done() (lines 1333-1337), builds a fresh session (lines 1343-1352), and re-Puts the key (line 1359) for a reader that is already closed. That new session hangs off an entry no longer in readerTempSessions, so neither DeleteReaderTempInfo nor the readerTempSessions.Range in provider.Close() (lines 1499-1509) can ever reach it. Failure: one goroutine triggers UpdateReaderTempInfo from ReadNext (entry loaded, lock not yet taken) while another calls reader.Close() → DeleteReaderTempInfo runs to completion → the update rewrites the deleted key with a new session that nothing tracks; its keepalive renews until the process or etcd client exits, so the zombie key permanently pins the truncated-segment cleanup low-water mark and storage is never reclaimed. This interleaving is reachable in supported usage: logBatchReaderImpl.Close (woodpecker/log/log_reader.go:240-257) only reads fields fixed at construction (logHandle, logId, logName, readerName, logIdStr, logNs), while ReadNext writes a disjoint set (batch, next, lastRead, pendingRead*, currentSegmentHandle, lines 128-229 and 299-357), so "read in one goroutine, close from another" is not itself a data race at the reader layer. Suggestion: set a closed flag under the entry mutex, and after acquiring the lock re-validate that the map still maps this key to the same, non-closed entry before recreating a session or writing; the existing test at meta/metadata_etcd_reader_session_test.go:141-153 is sequential and should be extended with a deterministic concurrent interleaving. (raised by nsj)

Medium

• meta/metadata_etcd.go:1161 — concurrency.NewSession gets no bounded context, so CreateReaderTempInfo ignores the caller's deadline when etcd is unreachable. concurrency.NewSession(e.client, concurrency.WithTTL(...)) is called without concurrency.WithContext, so etcd's NewSession performs its Grant on client.Ctx(), which is cancelled only when the etcd client itself is closed; combined with clientv3's default grpc.WaitForReady(true), the RPC queues instead of failing fast. Failure: etcd is unreachable or partitioned and the application calls OpenLogReader → CreateReaderTempInfo with a bounded deadline → Grant blocks indefinitely on client.Ctx(), the caller's deadline and cancellation are ignored, and OpenLogReader hangs until etcd recovers or the client is closed. This is a regression relative to the previous lease, err := e.client.Grant(ctx1, 60) under e.getContextWithTimeout(ctx), which returned ErrMetadataWrite after e.requestTimeout and let the caller retry or degrade. The same pattern appears in the update path at line 1343. The pre-existing writer-lock call at line 686 shares the shape, but that is a separate path and does not offset losing the timeout here. Suggestion: pass concurrency.WithContext(ctx1) (or at least a context bounded by e.requestTimeout) at both 1161 and 1343. (raised by xiaocai)

Low

• meta/metadata_etcd_reader_session_test.go:84 — 8.5s unconditional sleep with no testing.Short() skip, in a package that make unit-test runs with -short. testReaderTempInfoOutlivesLeaseTTLWhileIdle lowers the TTL to 3s and then sleeps TTL*2.5 + 1s = 8.5s, with no if testing.Short() { t.Skip(...) } guard. It is registered under TestAll at meta/metadata_etcd_test.go:105 and uses embedded etcd (common/etcd/etcd_util.go:44-46), so it is inside the make unit-test (go test -race -short -cover -failfast) package set, which excludes only tests/{integration,benchmark,stability,docker}. Per the repo house rules, long tests should honour testing.Short(). A 3s TTL with a 1s heartbeat also leaves thin margin on a loaded -race runner, and with -failfast a flake truncates the rest of the suite. Suggestion: add the testing.Short() skip and raise the TTL to 5s or more. (surfaced during verification)

[tinswzy]

@czs007 Replying to the re-review of 97c15df802df:

All four findings confirmed and addressed in c2874da, each with a regression test that was verified failing on the previous commit first:

• Failed Put revoking the healthy lease (High) — the retry loop now rotates the session only when the lease itself is gone (rpctypes.ErrLeaseNotFound on the Put, or the session's Done() channel already closed). Unrelated failures (caller cancelled, etcd slow) return the error without touching the session. Regression test: test update put failure does not revoke reader temp info lease (update with a dead caller context must leave the key attached to a live lease).

• Update/Delete TOCTOU resurrection (High) — entries are now marked closed under their mutex on every retirement path (DeleteReaderTempInfo, same-name reopen in CreateReaderTempInfo, provider Close()), and the update re-validates after acquiring the lock that the entry is non-closed and still the map's current entry for that reader. The post-lookup body was extracted to updateOwnedReaderTempInfo so the test drives the exact interleaving deterministically: test update racing with close does not resurrect reader temp info.

• Unbounded concurrency.NewSession (Medium) — fixed, but deliberately not with concurrency.WithContext(ctx1): the session's keepalive context derives from the option context, so a request-scoped context would kill the keepalive the moment the request ends and quietly reintroduce the original expiry bug (WithContext's own doc notes the lease is then abandoned to expire). Instead, newReaderTempSession grants the lease under the request-scoped/bounded context and attaches the session to it via WithLease, keeping the keepalive on the client's lifetime context. Regression test: test create reader temp info honors caller deadline when etcd unreachable (previously hung indefinitely; now fails within the deadline).

• Missing testing.Short() guard (Low) — the idle-reader test now skips in short mode and uses a 5s TTL for more margin under -race.

Verified: go test ./meta/ (full TestAll, including the pre-existing self-heal test which now exercises the ErrLeaseNotFound rotation path), go test -race -short ./meta/, go test ./woodpecker/log/, golangci-lint run ./meta/... — all green.

Known boundary unchanged: a concurrent same-name reopen while an update is mid-flight can still transiently drop the key until the next update (real reader names are unique per open, so this is not reachable in supported usage); it is narrowed but not eliminated by the closed-flag gate.