fix(telegram): fence staging-temp reaping to dead publishers and exact identity - #3654
Conversation
a607df0 to
dac2fcf
Compare
|
Canonical dev advanced to 1fbd7d4 immediately after this successor opened, so the exact-head plan correctly rejected the prior head. I rebased again onto that exact dev tip and force-pushed current head |
The notification self-heal reaper never claimed `.tmp` staging files, so every failed publication left one unreachable file in the agent notifications directory permanently. `writeJsonAtomic` stages a sibling `<name>.<pid>.<epoch-ms>.<suffix>.tmp` and renames it over the destination. If the staging write or the rename fails, or the process dies between the two, that temp is never published and never read again. No prefix in NOTIFICATION_LEAK_ARTIFACT_PREFIXES matched it, so the reaper walked past it. This accumulates across the roots registry, daemon state, callback aliases, seen-update ids, and the topic registry snapshot, and is most visible where a rename-blocking condition persists (Windows EPERM from an antivirus or indexer handle, EACCES, EIO, ENOSPC). Reaping is shape-matched and still bounded by the existing five-minute mtime grace window, so a temp an in-flight publication is still staging is never removed. Fixing it in the reaper rather than the writer also reclaims temps orphaned by a crash, which no writer-side unwind can reach, and leaves the protected `writeJsonAtomic` lifecycle declaration byte-identical so no DAEMON_GENERATION bump is required.
…t identity The staging-temp reaper decided purely on filename shape plus a path-following mtime, so a live-but-blocked publisher whose temp aged past the grace window could have its in-flight file deleted, and the readdir/stat -> unlink gap allowed an ABA replacement or a symlink to be destroyed by pathname. Reaping now requires a proven-dead publisher and an identity-bound delete: - The publisher pid is parsed from the writer name shape the writer already produces, and only a probe result of `dead` proceeds. `alive` and any indeterminate or throwing probe retain the file and count as skipped. - Identity is captured no-follow via the existing `readEndpointFile` seam, multi-link files are rejected, and removal goes through `exactUnlink`, which verifies dev+ino+size+mtimeNs+sha256 before unlinking. The grace decision uses the captured `mtimeNs`, so the same inode is bound from decision through deletion. - Missing seams retain rather than fall back to an unfenced unlink, and the staging quarantine name is added to the leak-artifact prefixes so a retained quarantine self-heals instead of becoming a new leak. No protected declaration changes, so no DAEMON_GENERATION bump is required.
dac2fcf to
1e28b82
Compare
|
Rebased onto current |
|
All exact-head CI checks are now terminal and green on current dev. The branch remains unchanged at 1e28b82. A fresh review would be appreciated when convenient. Thank you. |
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Hostile disposition: MERGE_READY
Latest exact head 1e28b820a0501815cc066fe022d1dd03eea647f7; exact base is current dev@71e9c95d9abdd9a96d223f815f6be29d47617e76. The stale-base run 30630512033 was not used as verdict authority; the successor head has fresh terminal CI: 17 green, 0 red, 0 pending, with 5 expected skips.
Direct predecessor P1 mapping: abandoned staging temps require dead-publisher proof, grace age, no-follow endpoint identity, single-link evidence, and native exact-unlink authority. Retained cleanup_pending results are not reported as terminal removal: the code requires a concrete detached path and canonical-path absence, reports the artifact as skipped, and preserves the detached payload under a recognized exact-unlink placeholder. Subsequent scans explicitly retain recognized placeholders without pathname churn. Symlink, same-name ABA replacement, live/unknown publisher, malformed claim, and retained-quarantine replacement regressions are covered by the changed test suite; exact CI passed the staging test, shard-1, native-build, TypeScript build, generation guard, Windows Telegram safety, evidence producer, and state gates.
Protected #3596 native source is not touched; only the shared changelog path overlaps. No P0/P1 found. Local reproduction was not claimed because this detached worktree lacks installed workspace package links; exact-head CI is the authoritative test evidence.
Exact head/base, terminal CI, and non-overlap gates are satisfied. Merge is authorized.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
2 participants
Thank you for the precise review on #3614. Both P1 findings are repaired, and I found that neither actually required coordination with #3596 —
exactUnlinkand the no-followreadEndpointFilecapture already exist on dev and are already imported by this very file, and the publisher identity you asked for is already present in the staged name the writer produces. So the repair stays entirely inside the unprotected reaper, and noDAEMON_GENERATIONbump or guard regeneration is needed.P1-1 — reaper was not publisher-liveness bound
You were right that shape + mtime cannot distinguish a blocked-but-live publisher from crash residue. Reaping now requires a proven-dead publisher:
parseNotificationStagingTempextracts{destination, pid, stagedAtMs}from the writer's own name shape, gated by the existingvalidDaemonPid, so pid 0, overflow, and non-integer values yield no claim.classifyNotificationStagingPublisherreturnsalive | dead | unknown, and maps a throwing probe tounknown.deadproceeds.alive,unknown, and no-claim all retain and count asskipped.This is a different mechanism than the generation encoding you suggested, and I want to be explicit about why: encoding a generation into the staged name would change
writeJsonAtomic, which is a protected declaration, requiring theDAEMON_GENERATIONbump and manifest regeneration that #3596 owns. The pid is already in the name, so the safety property you asked for — reap only proven-dead claims — is reachable without touching the writer at all. If you would still prefer the generation-encoded form, I am glad to do it once #3596 lands.P1-2 — pathname-only stat then unlink
Removal is now identity-bound end to end:
readEndpointFileseam, which rejects symlinks and directories (Endpoint is not a regular file) and concurrent mutation (Endpoint changed while it was read). Both propagate into the existing best-effort catch, so the file is retained.nlinkon thestatseam, failing closed when the seam or the field is absent.exactUnlinkAcceptedWithRetainedEvidencewith a distinct.gjc-delete-notification-staging-temp-<uuid>.jsonquarantine name, so the native verifiesdev+ino+size+mtimeNs+sha256before unlinking.identity.mtimeNsrather than a second path-followingstat, so the decision and the delete bind the same inode. This also removes a fractional-mtime negative-age hazard.NOTIFICATION_LEAK_ARTIFACT_PREFIXES, so a retained quarantine self-heals on a later pass instead of becoming a new permanent leak.If either seam is missing, the temp is retained — never an unfenced unlink.
Verification
Nine tests, public surface only, no
as anyand no private access. On unmodified dev 8 of 9 fail; with the repair 9 of 9 pass. Ran the file 8 consecutive times with zero flakes.New coverage for exactly the races you named: live publisher never reaped however old, indeterminate liveness retained, unparseable claim retained, dead-past-grace reaped through the identity fence, ABA replacement between capture and delete refused, and a symlink shaped like a staging temp never followed or deleted.
I also isolated each fix and reverted only it, with the other in place, to prove the coverage is specific rather than incidental:
The file was verified byte-identical to baseline after each revert/restore cycle.
Gates: 585 pass / 0 fail across five telegram suites,
tsc --noEmitclean,biome checkclean, andtelegram-daemon-generation-guard --validate-current-treeexits 0 with no generation bump demanded. Rebased onto current dev `7f5167c86`.One honest gap
The startup self-heal call site lives inside the guard-protected
rundeclaration, so threading an injectablepidAlivethere tripped the manifest digest check. I reverted that one line; startup self-heal uses the production probe, which is behaviorally correct, but that specific path is not test-injectable. Both exported entry points are fully injectable and every fence branch is covered through them.Fresh current-dev successor
This successor is rebased onto dev at 4f2805c after #3647 merged. Exact-head local verification on a clean worktree passed 525 tests across the staging regression and full Telegram daemon suite (0 failures, 2061 assertions), the generation guard reported v32 with no protected changes, and Biome reported both changed TypeScript files clean. CI is intentionally restarted on this exact head; no native authority source from #3596 is modified.
This replaces closed #3648. Thank you for reviewing the current exact head.