fix(telegram): make delete settlement transactional and overflow-safe - #3647
Conversation
…ep rollback route-atomic settleDelete() previously matched only topic id plus delete_pending, so a held generation's definite remote result could settle a newer re-fenced delete for the same session and release its topic-id quarantine. It now requires the caller's dispatched authority epoch to equal both the record's epoch and the registry's current epoch, and all four daemon call sites capture that epoch before dispatching the remote delete. restoreDeleteFence() reinstated a pending collision without rebuilding the derived routing tables, and its narrow byTopic.delete never ran on the absent-record branch that settlement creates. It now rebuilds inbound routes after writing the epoch, so a restored fence re-quarantines the topic id instead of leaving inbound routing open to the collision partner. Settlement also rebuilds routes on success so a settled topic id becomes adoptable and a surviving colliding record becomes routable immediately.
Settlement is now a two-phase commit. `settleDelete` removes the record, records the pending settlement, and retains the topic-id quarantine; `commitSettledDelete` publishes routes only after the clear is durable, and `rollbackSettledDelete` undoes it under a CAS on the exact post-settlement state. Epoch advancement saturates at the safe-integer bound and a saturated fence refuses settlement, so two generations can no longer collapse onto the same double. The first revoked-lease compensation sets its completion flag only after the durable commit, so a failed clear persist leaves the fence supervised instead of stranding cleared memory against a `delete_pending` disk state.
|
This is the exact-current-base successor to closed #3618. GitHub would not reopen #3618 after its requested force-push/rebase ( |
|
Thank you — the independent hostile review found a real saturated-epoch CAS gap, and successor head
New deterministic regressions cover active-at-MAX → delete fence → concurrent re-fence → stale restore (restore false, still pending, unroutable/unadoptable) and saturated creation refusal without callback invocation. Exact-head verification: 522 focused topic-registry/daemon tests passed, 0 failed; Biome clean; declaration checks clean; daemon generation guard green. |
Independent exact-head hostile review: APPROVEReviewed clean exact head
Independent recommendation: APPROVE. Reviewer ran no gates; caller verification on this exact head is 522 focused tests passing, Biome clean, declarations clean, and generation guard green. |
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Hostile disposition: REQUEST_CHANGES
Fresh-base review against dev@66475d50377073cd2e028da0a0377964babbaf45. The originally requested exact head 7d8200416 is no longer the current PR head; GitHub now reports a10c6dec23dd1414230ca8a0c0c2e0ea44a5668c, so the requested exact-head gate is not satisfied.
Direct predecessor P1 mapping: the epoch-bound two-phase settlement, stale-settlement refusal, rollback CAS, saturation fail-closed behavior, quarantine retention, and compensation supervision are implemented in topic-registry.ts / telegram-daemon.ts, with registry tests covering those state invariants. However, the admitted missing P1-4 regression remains material: this is a daemon-level compensation contract, not only a registry contract. The added daemon test proves a concurrent re-fence during deleteTopic, but does not exercise the first-create compensation path through remote compensation success followed by failed durable clear, nor assert that the fence remains supervised and is durably replayable. That observable proof is required; green CI and registry-only assertions are insufficient.
No merge or build performed. Re-establish the exact reviewed head/base, add a daemon-level failed-persist compensation regression covering the externally observable state and durable replay, then obtain terminal exact-green CI and a fresh signed P0/P1-zero review.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
|
Thank you. Successor head The deterministic regression forces:
It asserts the exact externally observable disk state after the failed clear ( Exact successor verification: 523 focused registry/daemon tests passed, 0 failed; new test passes independently; Biome clean; declarations clean; daemon generation guard green. The prior exact-head mismatch is superseded by this explicitly mapped current head. |
|
Independent hostile successor verdict: APPROVE (P0=0, P1=0) Reviewed clean exact head The new daemon regression ( Settlement remains epoch-bound/two-phase with retained quarantine and CAS rollback ( No P0/P1 findings. Reviewer ran no gates; caller exact-head verification is 523 focused tests passing, Biome clean, declarations clean, and generation guard green. — Architect |
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Hostile disposition: REQUEST_CHANGES / BLOCKED_PENDING
Latest exact head reviewed: 2f1493aa48173e0d7340185332a44307ffc4f2a0; exact base: dev@66475d50377073cd2e028da0a0377964babbaf45. I did not use the prior 7d8200416 evidence as verdict authority.
The Telegram assertion set is not the source of the red check: the Windows Telegram job reports all 40 selected Telegram tests passed. The failure is packages/natives/test/native.test.ts:124 (pi-natives > signals only the pinned root process), where child enumeration returned false after approximately 64ms. Current dev has a separate same-head successful workflow 30626393686, so this is presently classified as a likely native Windows timing/inherited flake, not a Telegram regression. I am not authorizing a rerun or asking for Telegram-code changes for this unrelated failure.
The PR remains blocked: required exact-head CI is not terminal green, and Telegram daemon generation guard is still pending. No merge or build is authorized. A bounded native-flake authority or a fresh exact-head terminal-green result is required before any MERGE_READY consideration. The changed paths are Telegram-only and do not overlap protected #3596 native source; the dev red roots are separate session-owner/NotificationServer/Bash surfaces.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
|
CI retry note: the prior exact head Admin-only job rerun is unavailable, so I pushed empty successor
All prior source verification and independent P0/P1-zero mapping therefore apply to identical content; terminal exact-head CI remains required. |
|
Independent exact-head hostile approval: P0=0, P1=0 Exact head The unchanged tree retains the reviewed saturation-safe epoch settlement, tokenized two-phase commit/rollback, durable-clear compensation supervision, route/quarantine invariants, and daemon restart replay coverage. No new P0/P1 findings. APPROVE exact — Architect |
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Hostile disposition: MERGE_READY
Latest exact head aa9abe51579e0fe6920f921de66b33dab291e2a3; exact base dev@66475d50377073cd2e028da0a0377964babbaf45. The intervening commit is a CI-only retry with an identical source tree to 2f1493aa48173e0d7340185332a44307ffc4f2a0; no prior 7d8200416 evidence was used as authority.
Direct P1 mapping is complete. The daemon-level first-create compensation regression now proves remote compensation followed by failed durable clear remains supervised and durably replayable. The concurrent re-fence test proves a definite remote delete cannot settle a newer fence. TopicRegistry tests cover stale settlement refusal, two-phase quarantine retention, failed-persist rollback CAS, saturation fail-closed behavior, route restoration, and refused-settlement token absence. Exact focused verification on this latest tree passed: 512 tests, 0 failures, 1,983 assertions.
The formerly red Windows check is now green after the contributor's CI-only retry commit; all required exact-head checks are terminal green: affected validation, shard-1, native-build, TypeScript build, CLI smoke, generation guard, Windows Telegram safety, evidence producer, state gates, and public-surface checks. No P0/P1 found. Changed paths are Telegram-only and do not overlap protected #3596 native source; dev red roots are separate session-owner/NotificationServer/Bash surfaces.
Exact-head/base gate is satisfied. Merge is authorized for this exact non-overlapping candidate only.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
|
All exact-head checks are now terminal and green (successful or intentionally skipped), and the independent hostile review at the current head found P0=0/P1=0. The aggregate CHANGES_REQUESTED verdict is tied to the superseded predecessor head. When convenient, could you please take a fresh look at the current head? Thank you. |
Successor to #3617. Thank you for the four-finding review — each one is repaired below, with an isolated-revert proof per finding.
Base: current dev
7f5167c86.P1-1 — epoch overflow
All advancement now routes through a saturating helper (
current >= Number.MAX_SAFE_INTEGER ? MAX_SAFE_INTEGER : current + 1), wired intobeginDelete,delete, and both restore paths.settleDeleterejects non-safe-integer and negative dispatched epochs, and refuses settlement outright once a session's epoch has saturated — past that bound two distinct generations collapse onto the same double, so the comparison is no longer sound and the fence is retained instead.P1-2 — rollback must CAS, and a refused settlement must not restore
settleDeletenow returnsTopicSettledDelete | undefinedinstead of a boolean. A refused settlement returns no token, so it is structurally incapable of reaching a rollback — enforced by the type, not a runtime guard.rollbackSettledDelete(settled)applies only while the post-settlement state is still exactly current: the settlement is still pending in#settlingwith matching topicId and epoch, the session still has no record, and the current authority epoch still equals the settled epoch. Any mismatch refuses and leaves the newer state untouched. Rollback restores at the settled epoch exactly rather than bumping, since a CAS that asserts nothing moved must not itself move.P1-3 — routes published before the clear is durable
Two-phase commit. Phase 1 removes the record, records the pending settlement, and retains the topic-id quarantine;
rebuildInboundRoutes()re-quarantines every pending settlement, so the quarantine survives any rebuild triggered during the held write. Phase 2 (commitSettledDelete) is the only place the quarantine is released and colliding survivors become routable. All four call sites capture the token, persist, then commit on success or roll back on failure.P1-4 — first compensation marked complete before clear persistence
acceptedTopicCompensated = truemoved from before the persist into the post-persist success branch, immediately after the phase-2 commit. That flag is what tells outer recovery to stop supervising, so a failed clear persist now leaves the fence supervised instead of stranding cleared memory against adelete_pendingdisk state. The persist is wrapped locally so a persist failure does not mask the in-flight revocation error the outer catch depends on.Verification
Each finding was proven by reverting only that fix with the other three in place:
authority epochs saturate at the safe-integer bound…Expected: 9007199254740991/Received: 9007199254740992a rollback refuses any settlement whose post-settlement state no longer holdsExpected: false/Received: truea settled delete keeps its topic id quarantined until the clear is durableExpected: false/Received: trueAll 7 tests in the file fail on unmodified dev and pass with the fix. Wider run:
notifications-topic-settle-fence-epoch+notifications-topic-registry+notifications-telegram-daemon→ 572 pass / 0 fail. Generation guard,tsc --noEmit, and biome all exit 0; no generation bump is demanded because no protected declaration changed.One honest gap
P1-4's fix is applied, but I could not build a regression for it. The hazard is an ordering constraint over a function-local
letinside the daemon's create callback, observable only through whether two later daemon-internal recovery blocks supervise the fence — it produces no distinguishableTopicRegistrystate. I verified this empirically: reverting the P1-4 fix with the other three in place leaves all 60 tests green. Covering it needs a daemon-level harness. I'd rather state that plainly than claim coverage I don't have.No file owned by #3596 is touched (
telegram-daemon-contract.ts, the generation manifest,file-lock.ts,path_identity.rsall unmodified).