Skip to content

fix(lifecycle): address six retention and cleanup review regressions #1038

Closed

Description

@hrygo

PR #1036 review found six lifecycle regressions at baseline 2db214fb0b2b9ae46dba9afdb7d72c5fabed7486. Fix and validate them together on the existing PR.

Evidence: Source = current PR code; Test = isolated SQLite/Go overlay reproduction; Live = not exercised against production.

  • R1: Migrated sessions renew after accepted input. Migration and Session Manager share the revision encoding; renewal preserves historic revisions and longer deadlines, refreshes persisted lifecycle fields, and uses a persisted-revision CAS. Real migration and hot/cold manager regression tests pass.
  • R2: Timeout no longer mutates forwarder-owned aggregation. Real forwarding/callback tests verify one terminal response and pass race checks. The original isolated overlay reproduced the concurrent accesses; repeated baseline runs of the real integration test did not reliably reproduce the race.
  • R3: Unregistered Worker cleanup returns ErrSessionCleanupUnsupported. SQLite/PostgreSQL atomically record an unsupported purge item and a blocked aggregate status; stale leases and repeated deletes cannot overwrite or recreate the task. Registered cleanup and retry tests pass.
  • R4: Paired migration 045 persists immutable retention windows and policy revisions when effect/input records are created. GC uses the stored window plus the settlement timestamp. Configuration changes affect new records; legacy NULL windows remain protected. SQLite cohort tests and the real PostgreSQL regression cover both acceptance paths, the 90-day BIGINT window, duplicate stability and GC boundaries.
  • R5: First durable queue acceptance records lifecycle activity before ACK; duplicates do not renew activity. A lifecycle failure cancels the row and returns INPUT_LIFECYCLE_UPDATE_FAILED without volatile fallback. Additional same-ID retry regressions verify that lifecycle failures are not ACKed as queued and cancelled queued records report their actual terminal status, including the cached Handler path.
  • R6: Bounded migration candidates and readiness now use the same maximum deadline rule. Batch-size-one regressions verify convergence after raising legacy retention, without shortening existing deadlines.

The six review fixes are delivered by commit 15986c5591dd4610de36c0bfca5cf3301daed392, followed by the PostgreSQL queue budget placeholder fix in 8cfa8a763c59bbfa9a92183650d58146b1a77cf3, dispatch/settlement/depth fixes plus regression coverage in 8a622f448ec15d70a54b0f507eb8bad2af37e6c4, effect/operator settlement type fixes in 6295259228b05ab0e66bec2e9a19ac27a62c597b, lifecycle failure-safety coverage in ae03a6c04dcaeab63de39b38c5b7b9ce653ffe9d, and serialized/bounded Repairer shutdown in 5f59804cceae18e9fc1c5da84ea78cfeb78e2c20, on PR #1036. Detailed evidence is recorded in docs/issues/2026-10-data-lifecycle/PR-review-fixes.md.

The first real PostgreSQL CI run exposed a previously untested queue budget mirror update that sent a SQLite ? placeholder directly to PostgreSQL. The finding was registered before the fix in this comment. The incremental fix uses the existing dialect rebinding; the regression test was retained unchanged.

The second real PostgreSQL run advanced through acceptance, then exposed the same omission during claim. A broader source check found the shared settlement deletion and session-depth query too. These were registered before fixing in this comment. All three use the existing dialect helper; the PostgreSQL test now also verifies claim depth changes, cancel/clear/expiry terminal states and retention snapshot stability.

The third real PostgreSQL run completed queue operations and input-retention assertions, then exposed a text/BIGINT inference error in the effect completion settlement CASE. The same pattern existed in operator actions. Both timestamp arms now have explicit BIGINT casts; the finding was registered before the fix. Coverage was extended to unknown delivery, operator terminal decisions and requeue clearing the settlement clock.

The fourth run passed ordinary tests and the real PostgreSQL regression, but session coverage was 69.8%, below the existing 70% gate. The gap was registered before adding deterministic failure-safety tests. They verify failed/refused retirement releases cached/cold deletion fences, renewal persistence errors cannot report success or change deadlines, deleted sessions cannot be revived, and legacy inputs do not acquire v2 deadlines. Local full session tests with race/shuffle/coverage passed at 72.6%. This incremental commit changes tests and the ledger only; thresholds are unchanged.

The fifth run passed PostgreSQL and the unchanged coverage gate (Linux session 70.6%), but caught a real Repairer shutdown race: background processing and shutdown draining operated on the same mutable intent. The finding was registered before a channel-controlled regression reproduced both overlapping store calls and race reports. Shutdown now has one teardown owner, waits for the loop before draining, and does not drain after a cancelled/timed-out wait. The drain has a bounded context. Active-loop, drain-only, timeout and cancellation scenarios passed 10 consecutive race/shuffle rounds.

Verification on October 9, 2026:

  • Local full make quality passed: lint reported zero issues and the full Go test suite passed with -race -count=1 -shuffle=on.
  • make build and make docs-lint passed. Commit/push hooks ran without bypass; all six pre-push quality gates passed.
  • The PostgreSQL regression compiled locally but was skipped because HOTPLEX_TEST_PG_DSN was absent; the PR CI supplies a disposable PostgreSQL 18 database.
  • Production runtime and data were not modified.

Final CI verification on October 9, 2026:

  • Current PR HEAD: 5f59804cceae18e9fc1c5da84ea78cfeb78e2c20.
  • CI run 37874490089 completed successfully for this exact HEAD. Every job passed, including the race/coverage test suite, real PostgreSQL regression, Coverage Check and Validation Gate.
  • Both PR Checks runs for this HEAD passed branch naming and issue linkage.
  • Local worktree is clean. The fixes are pushed to PR feat(lifecycle): separate runtime and history retention #1036; merge and production rollout remain outside this task.

Activity

  1. hrygo commented on Oct 9, 2026

    @hrygo
    OwnerAuthor

    R5 修复复审补充(2026-10-09,Source):

    基准仍为 2db214fb,发现位于当前未提交修复。首次队列接受后若 lifecycle 更新失败且 CancelQueued 成功,same-ID 重试会得到已 failed 的 duplicate record。EnqueueBusyInput 忽略队列 entry 并返回 queued=true,导致错误的 durable queued ACK。

    验收:真实 SQLStore 下首次失败与 same-ID 重试都不能承诺排队;记录保留原失败状态和 ID,不复活队列、不续期、不进入 volatile buffer。新增 RED 后在本次同一修复提交解决。

  2. hrygo commented on Oct 9, 2026

    @hrygo
    OwnerAuthor

    CI exposed an additional PostgreSQL queue acceptance bug while running the new retention regression on commit 15986c5591dd4610de36c0bfca5cf3301daed392.

    • Test evidence: TestPGRetentionSnapshotSurvivesPolicyShortening failed at the first AcceptQueued call with execution: refresh queue budget mirror: ERROR: syntax error at or near "WHERE" (SQLSTATE 42601).
    • Source evidence: internal/execution/sql_queue.go passes UPDATE execution_queue_budget SET used = ? WHERE budget_id = 1 directly to PostgreSQL, without the existing dialect placeholder rebinding.
    • CI evidence: https://github.com/hrygo/hotplex/actions/runs/37866767486/job/113615275975
    • Acceptance: use the existing dialect rebinding for this update; retain the queue budget lock/transaction semantics; the unchanged real PostgreSQL retention regression must pass, including first queued acceptance.
    • Delivery: an incremental fix on PR feat(lifecycle): separate runtime and history retention #1036. No production database or configuration changes.
  3. hrygo commented on Oct 9, 2026

    @hrygo
    OwnerAuthor

    The second PostgreSQL run on 8cfa8a763c59bbfa9a92183650d58146b1a77cf3 passed the first queue acceptance, then the unchanged retention regression exposed another dialect omission at ClaimQueued.

    • Test: TestPGRetentionSnapshotSurvivesPolicyShortening fails with execution: remove claimed queue row: ERROR: syntax error at end of input (SQLSTATE 42601).
    • Source: internal/execution/sql_queue_dispatch.go has three direct parameterized statements without s.rebind: queue-row removal after claim, queue-row removal during settlement (cancel/clear/expire), and per-session queue depth.
    • Evidence: https://github.com/hrygo/hotplex/actions/runs/37867885313/job/113618965463
    • Acceptance: rebind all three statements using the existing dialect helper. Extend the real PostgreSQL regression to cover per-session depth and cancellation/clear/expiry in addition to the existing claim and retention checks. SQLite and PostgreSQL must preserve atomic queue/runtime transitions.
    • Scope: incremental delivery on PR feat(lifecycle): separate runtime and history retention #1036; no production runtime or database operations.
  4. hrygo commented on Oct 9, 2026

    @hrygo
    OwnerAuthor

    The third PostgreSQL run on 8a622f448ec15d70a54b0f507eb8bad2af37e6c4 advanced through input retention and queue operations, then failed while settling a delivery effect.

    • Test: TestPGRetentionSnapshotSurvivesPolicyShortening fails at CompleteSend with column "settled_at" is of type bigint but expression is of type text (SQLSTATE 42804).
    • Source: applyCompletionSQL has CASE ... THEN ? ELSE NULL END; PostgreSQL resolves the unknown parameter/NULL result to text rather than the BIGINT destination. operatorUpdateSQL has the same pattern.
    • Evidence: https://github.com/hrygo/hotplex/actions/runs/37869069018/job/113622690480
    • Acceptance: explicitly cast the timestamp arm to BIGINT in both statements, retaining existing settlement/requeue semantics and SQLite compatibility. Real PostgreSQL coverage must complete delivery settlement/GC and cover operator terminal/requeue timestamps.
    • Scope: incremental repair on PR feat(lifecycle): separate runtime and history retention #1036; no production database or runtime changes.
  5. hrygo commented on Oct 9, 2026

    @hrygo
    OwnerAuthor

    The real PostgreSQL regression and ordinary tests now pass on 6295259228b05ab0e66bec2e9a19ac27a62c597b; the remaining failure is the existing session package coverage gate.

    • CI reports internal/session at 69.8%, below the unchanged 70% threshold.
    • The downloaded coverage profile contains 2,115 covered statements of 3,028 (69.85%). Final rounding in the gate is 69.8%.
    • Useful uncovered lifecycle boundaries include retirement-store failure/CAS refusal and restoring cached/cold session availability after an unsuccessful retirement, plus renewal persistence failures.
    • Acceptance: add deterministic regressions for these lifecycle safety behaviors and verify package coverage with race/shuffle. Do not lower thresholds or bypass the gate.
    • Evidence: https://github.com/hrygo/hotplex/actions/runs/37870416313/job/113629331683
    • Deliver test-only changes and the evidence ledger on PR feat(lifecycle): separate runtime and history retention #1036.
  6. hrygo commented on Oct 9, 2026

    @hrygo
    OwnerAuthor

    Final-head CI on ae03a6c04dcaeab63de39b38c5b7b9ce653ffe9d passes PostgreSQL and coverage (session 70.6% on Linux), but detects a real Repairer shutdown race.

    • Test/CI: race detector reports concurrent access to RepairIntent.attempts at repairer.go:244/262 in TestRepairer_SuccessfulProcessing. The two paths are loop → processDue → tryProcess and Shutdown → drain → processDue → tryProcess.
    • Source: Shutdown signals the loop, starts draining immediately, and only then waits for the loop. Concurrent Shutdown callers can also start simultaneous drains because stop.Do protects only channel closure.
    • Evidence: https://github.com/hrygo/hotplex/actions/runs/37872296025/job/113632938359
    • Acceptance: one shutdown owner; wait for the processing loop before draining; timeout/cancellation must not introduce a second processor. Channel-controlled regressions must prove no duplicate delivery-store call/success hook for active-loop and drain-only shutdown, and race/shuffle tests must pass.
    • Deliver the incremental repair on PR feat(lifecycle): separate runtime and history retention #1036. No production operation, protocol change or retention-policy change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions