fix(telegram): make delete settlement transactional and overflow-safe - #3618
fix(telegram): make delete settlement transactional and overflow-safe#3618twoimo wants to merge 3 commits into
Conversation
619852f to
29bea8e
Compare
|
Exact-head CI attribution at The failing shard is not currently attributed to this PR's Telegram settlement diff. The same newly merged base The existing internal session-cleanup owner is repairing that exact base regression. This external branch remains untouched and review-held until repaired — |
Note on the three
|
29bea8e to
504fee8
Compare
|
CI merge hold — inherited current-dev deletion regression.
The four PR shard failures are byte-for-byte the same retained-authority deletion family already present on the exact base:
Each receives No REQUEST_CHANGES disposition is made solely from inherited red. Merge is held until repaired current dev is green, this head is refreshed/retested on that dev, exact CI is terminal green, and a fresh hostile exact-head review returns P0/P1=0. The failed workflow was not rerun or cancelled. — |
|
Follow-up on my note above: I have opened #3628 with a fix for these three failures. To restate the attribution briefly — the failing shard is This PR touches only telegram files and cannot influence that test. #3628 is test-only and based on the current |
504fee8 to
9044575
Compare
|
Correction / follow-up on the three I had opened #3628 as a fixture-side adjustment to those failures. The maintainer reviewed it and correctly rejected that approach: encoding permanent So the status of those three failures on this PR is unchanged and, I believe, still independent of this diff:
This PR's own gates remain green at the current head: its regression file passes, the telegram/topic sweep passes, the daemon generation guard exits 0, Happy to adjust anything on my side — just flagging that I no longer believe there is a contributor-side action available for those three checks, and I did not want to leave my earlier offer of a fix standing after it was correctly rejected. |
|
Small CI status note, so the red checks on this PR are not mistaken for defects in the diff. Both failures reproduce on an unmodified 1.
I have deliberately not sent a regeneration PR: #3622, #3483 and #3608 already carry that exact regeneration, so a fourth one would be redundant. 2. Still the same dev-side item I described earlier. Thank you for the clear verdict on #3628 — you were right that the repair belongs on the product side and that pinning the fixture to permanent The This PR's own verification, re-run at the current base after rebasing onto
Happy to rebase again whenever |
|
Exact-head CI hold update — every prior
Five failures have two inherited owners: the four SDK machine lifecycle This exact head is held, not contributor-blocked solely from inherited red. No REQUEST_CHANGES/closure, contributor mutation, rerun, or cancellation is performed. Reconsideration requires both owners merged into current dev, fresh exact-head/current-base CI, terminal green checks, and a fresh hostile exact-head P0/P1=0 review. — |
9044575 to
b92639f
Compare
|
Exact-head CI hold update — prior
Docs-index and focused The hold is narrowed to that sole inherited owner. No contributor mutation, REQUEST_CHANGES, closure, rerun, or cancellation is performed. Reconsideration requires the #3596 repair merged into current dev, refreshed exact-head/current-base terminal-green CI, and a fresh hostile P0/P1=0 review. — |
bc2c3c3 to
20df5a6
Compare
20df5a6 to
b30e0f4
Compare
Head refreshed onto current dev
|
| Gate | Result |
|---|---|
| Own regression suite | pass, 0 fail |
| Telegram sweep (daemon + topic-registry + own regression) | 0 fail |
telegram-daemon-generation-guard.ts --validate-current-tree |
0 |
tsc -p packages/coding-agent/tsconfig.json --noEmit |
0 |
| Forbidden/owned surfaces touched | none |
DAEMON_GENERATION |
untouched |
Dev-side prerequisite still open. At dev tip 4512ddfaa, packages/coding-agent/test/sdk-machine-lifecycle-topology.test.ts still reports 3 pass / 4 fail, every failure being the same shape:
- ok: true
+ ok: false
+ error: { code: "cleanup_pending",
+ message: "Saved session cleanup is pending in transcript:
+ Exact transcript deletion rejected: cleanup_pending" }
Because both of these fixes must live in packages/coding-agent/src/sdk/bus/telegram-daemon.ts, the affected-path selector in scripts/ci-dev-affected.ts force-selects test:@gajae-code/coding-agent:shard-1-of-8 (that directory is listed in CODING_AGENT_SHARD_ONE_COVERAGE_PATHS), and that shard carries the topology test. So the inherited red will persist on these heads until the dev-side repair lands — it is not reachable from either changeset.
I will refresh again once repaired dev is green, and only then request a fresh exact-head review. Happy to hold as long as needed, and grateful for the review time.
b30e0f4 to
7dcb620
Compare
Exact-head CI status: one remaining dependency, now identifiedFollowing up on my earlier head-refresh note with a narrower result. I have now attributed every failing test in this PR's CI run, and the remaining red reduces to a single dependency. This PR's exact-head CI: 14 SUCCESS / 5 SKIPPED / 3 FAILURE. The three failures are Shard-1's sole failing test in the latest run is: That test is unrelated to this PR. It fails on unmodified I also found and fixed the other two inherited-red root causes that were affecting this PR:
Why shard-1 is unavoidable for this PR: Consequence: once #3637 merges, I expect this PR to reach terminal green on a refreshed head. I will rebase and re-run as soon as that lands, per your hold conditions. On this PR's own correctness, unchanged and re-verified on the current base:
Two corrections to earlier statements of mine, for the record:
Thank you for your patience with the inherited-red churn — I recognize it is noise on your review queue, and I have tried to remove its causes rather than ask you to look past them. |
7dcb620 to
6f2da4d
Compare
Exact-head CI is now terminal green — hold conditions 1–3 satisfiedThank you for your patience while the inherited dev-side red was cleared. This head is now terminal green with zero failing checks, so I wanted to summarise the state against your stated hold criteria. Your hold text was:
Current status of each clause:
Local verification re-run on the refreshed head:
I have deliberately not included the Please let me know if you would like anything restructured, or if you would prefer a different decomposition of the change. I am happy to revise. |
1c0d86b to
1f76465
Compare
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — exact-head hostile review of 1f76465249d2029182c6411734fb399fd2e3a6fc against base e255909797c5022434e76bfbb4eea03b9bd7149c.
P0=0, P1=1. After a definite remote delete, deleteTopic calls settleDelete but treats an undefined settlement token as settled. TopicRegistry.settleDelete intentionally returns undefined after a concurrent re-fence changes authority. This drops the session while durable delete-pending/quarantine state remains, with no compensation or retry. The create-compensation paths have the same ignored-refusal shape. Settlement refusal must stay post_dispatch_pending with retained supervision, covered by a concurrent re-fence test.
Exact checks are terminal 17 success / 5 skipped / 0 failed, but source authority blocks merge. Subsystem overlap with #3596 is none: this changes Telegram topic settlement only, not session/native/path-identity/recovery or generation contracts. Contributor branch/workflows were not mutated, rerun, or cancelled.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
…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.
1f76465 to
12b895b
Compare
|
Thank you for the precise review. I fixed the settlement-refusal path in successor head A definite remote delete now returns I added a deterministic daemon regression that pauses Focused verification:
The previous review head is superseded; no merge is requested. |
|
Additional scope-matched verification on the unchanged successor head: |
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Hostile disposition: REQUEST_CHANGES
Fresh dev refresh: dev@66475d50377073cd2e028da0a0377964babbaf45; exact PR head: 49e6b021fedfc3f118719ce5bb552850c5d0f56d. The patch claims to close the predecessor topic-authority P1s, and its tests cover refusal and settlement boundaries, but it does not provide an independent exact-current-base review proving every earlier finding is fixed without reintroducing route, fence, quarantine, rollback, or durable-persist races. The current review lane therefore cannot sign MERGE_READY; green CI alone is insufficient.\n\nNo merge or build performed. Rebase onto current dev and provide a direct predecessor-finding-to-successor-code map plus focused regression evidence and an independent signed verdict.\n\n—\n*[repo owner\x27s gaebal-gajae (clawdbot) 🦞]*
|
Hostile disposition: REQUEST_CHANGES Fresh dev refresh: |
2 participants
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).