Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/coding-agent/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
- Runtime settings reconciliation now validates every `web_search.fallback` entry against the declared provider enum instead of accepting unsupported or non-string array items (#3601).
- Ultragoal critic-gate, dogfood, review, durable-completion, and runtime test suites now pin `CI_DEV_CHANGED_PATHS` hermetically in their setup/teardown. Their temp checkpoints live inside the enclosing git work tree, so the CI planner's changed paths (which include computer control surface paths on branches that touch them) previously leaked into the computed change set and falsely triggered the mandatory computer red-team suite (`COMPUTER_REDTEAM_CASE_MISSING: … must include kill-switch-bypass`). The production kill-switch-bypass gate is unchanged; only the test fixtures now isolate their own contract from the host branch's diff (#3533).
- Ultragoal critic-gate, dogfood, review, durable-completion, and runtime test suites now relocate temp dirs to `os.tmpdir()` (outside the enclosing git work tree) and pin `CI_DEV_CHANGED_PATHS` to a non-computer test path. The prior in-repo temp dirs caused `computeCheckpointChangeSet` to return `captureIncomplete=true` under parallel shard load (git command timeouts), which unconditionally triggered the mandatory computer red-team suite even when no computer surface was touched. The production kill-switch-bypass gate is unchanged; the `.tmp-*` gitignore entry prevents in-repo test artifacts from polluting untracked-file inventory (#3533).
- The Telegram notification self-heal reaper now reclaims abandoned publication staging files in the agent `notifications/` directory. `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 the reaper's leak-artifact list claimed `.tmp`, so one unreachable file accumulated per failed attempt — permanently, across the roots registry, daemon state, callback aliases, seen-update ids, and the topic registry snapshot. This is most visible where a rename-blocking condition persists (a Windows `EPERM` from an antivirus or indexer holding a handle, `EACCES`, `EIO`, `ENOSPC`). Reclaiming it here also recovers temps orphaned by a crash, which no writer-side unwind can reach. Removal is fenced rather than age-only: the reaper parses the publisher PID out of the temp's own name and removes it only when that publisher is *provably dead*, so a live or slow publication keeps its staged temp however old it is, and an indeterminate liveness probe or an unparseable claim retains the file. A proven-dead temp is still bounded by the existing five-minute mtime grace window, and the deletion itself is bound to a no-follow identity capture (`dev`+`ino`+`size`+`mtime`+content digest, single-link regular files only) executed through the exact-unlink native, so a symlink is never followed and a temp replaced between capture and delete is refused instead of destroying the successor.

### Fixed

Expand Down
188 changes: 176 additions & 12 deletions packages/coding-agent/src/sdk/bus/telegram-daemon.ts
Original file line number Diff line number Diff line change
Expand Up @@ -160,11 +160,17 @@ export interface TelegramDaemonFs {
size?: number;
dev?: number;
ino?: number;
/** Hard-link count; required to prove a staging temp has no second name. */
nlink?: number;
ctimeMs?: number;
isDirectory?: () => boolean;
}>;
readEndpointFile?(path: string): Promise<NotificationEndpointFile>;
exactUnlink?(path: string, identity: NotificationEndpointFileIdentity): Promise<NotificationExactUnlinkResult>;
exactUnlink?(
path: string,
identity: NotificationEndpointFileIdentity,
quarantineName?: string,
): Promise<NotificationExactUnlinkResult>;
}

export interface SpawnResult {
Expand Down Expand Up @@ -228,8 +234,12 @@ function negotiateToolActivityCapability(
const nodeFs: TelegramDaemonFs = {
...(fs.promises as unknown as TelegramDaemonFs),
readEndpointFile: readNotificationEndpointFile,
exactUnlink: async (file, identity) =>
exactUnlinkNotificationFile(file, identity, `.gjc-delete-daemon-transition-${crypto.randomUUID()}.json`),
exactUnlink: async (file, identity, quarantineName) =>
exactUnlinkNotificationFile(
file,
identity,
quarantineName ?? `.gjc-delete-daemon-transition-${crypto.randomUUID()}.json`,
),
};

/**
Expand Down Expand Up @@ -935,8 +945,9 @@ async function exactUnlinkAcceptedWithRetainedEvidence(
fsImpl: TelegramDaemonFs,
file: string,
identity: NotificationEndpointFileIdentity,
quarantineName?: string,
): Promise<boolean> {
const removed = await fsImpl.exactUnlink!(file, identity);
const removed = await fsImpl.exactUnlink!(file, identity, quarantineName);
if (removed.ok) return true;
return (
removed.code === "cleanup_pending" &&
Expand Down Expand Up @@ -1287,6 +1298,7 @@ export const NOTIFICATION_LEAK_ARTIFACT_PREFIXES = [
".gjc-delete-daemon-transition-",
".gjc-exact-unlink-placeholder-",
".gjc-delete-notification-endpoint-",
".gjc-delete-notification-staging-temp-",
] as const;

/** Grace window before a leak artifact is reaped (covers in-flight unlinks). */
Expand All @@ -1298,6 +1310,80 @@ export function isPermanentMissingPathError(error: unknown): boolean {
return code === "ENOENT" || code === "ENOTDIR";
}

/**
* Shape of an abandoned `writeJsonAtomic` staging file:
* `<published-name>.<pid>.<epoch-ms>.<base36-suffix>.tmp`.
*
* Publication stages a sibling temp and then 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
* {@link NOTIFICATION_LEAK_ARTIFACT_PREFIXES} claimed it, so it accumulated in
* the agent notifications directory permanently, once per failed attempt.
*
* Reaping it here (rather than only unwinding in the writer) also reclaims
* temps orphaned by a crash or power loss, which no writer-side cleanup can
* reach. Age alone is not proof of abandonment, though: a slow or blocked
* publisher (a stalled network write, a rename fenced by an antivirus handle)
* can hold a live staged temp well past any grace window. The staged name
* therefore carries its publisher's PID, and the reaper only removes a temp
* whose publisher is provably dead — see {@link parseNotificationStagingTemp}.
*/
const NOTIFICATION_STAGING_TEMP_PATTERN = /^(?<destination>.+)\.(?<pid>\d+)\.(?<stagedAt>\d+)\.[0-9a-z]+\.tmp$/;

/** A publication staging claim recovered from an abandoned temp's name. */
export interface NotificationStagingTempClaim {
/** Published sibling this temp was staged for. */
destination: string;
/** PID of the process that staged it. */
pid: number;
/** Wall-clock ms the publisher recorded when it staged the temp. */
stagedAtMs: number;
}

/**
* Recover the publication claim encoded in a staging temp's name, or
* `undefined` when `name` is not a staging temp or its PID/timestamp fields are
* not usable integers. An unparseable claim is never reaped.
*/
export function parseNotificationStagingTemp(name: string): NotificationStagingTempClaim | undefined {
const groups = NOTIFICATION_STAGING_TEMP_PATTERN.exec(name)?.groups;
if (!groups) return undefined;
const pid = Number(groups.pid);
const stagedAtMs = Number(groups.stagedAt);
if (!validDaemonPid(pid) || !Number.isSafeInteger(stagedAtMs) || stagedAtMs < 0) return undefined;
return { destination: groups.destination as string, pid, stagedAtMs };
}

/** Liveness verdict for a staging temp's publisher; `unknown` fails closed. */
type NotificationPublisherLiveness = "alive" | "dead" | "unknown";

/**
* Classify a staging temp publisher against the daemon's liveness seam. A
* throwing probe (an unreadable process table, a denied query) is
* indeterminate, not dead, so the temp is retained.
*/
function classifyNotificationStagingPublisher(
claim: NotificationStagingTempClaim,
pidAlive: (pid: number) => boolean,
): NotificationPublisherLiveness {
try {
return pidAlive(claim.pid) ? "alive" : "dead";
} catch {
return "unknown";
}
}

/**
* True when `file` is a regular file reachable under exactly one name. A
* multi-link file shares its inode with another pathname, so unlinking this one
* would not reclaim the data and may be another owner's live hardlink. Fails
* closed when the `stat` seam cannot report a link count.
*/
async function isSingleLinkRegularFile(fsImpl: TelegramDaemonFs, file: string): Promise<boolean> {
if (!fsImpl.stat) return false;
const stat = await fsImpl.stat(file);
return stat.nlink === 1;
}
export function isNotificationLeakArtifactName(name: string): boolean {
return NOTIFICATION_LEAK_ARTIFACT_PREFIXES.some(prefix => name.startsWith(prefix));
}
Expand Down Expand Up @@ -1408,21 +1494,88 @@ export async function pruneMissingNotificationRoots(input: {
return { pruned, remaining };
}

/**
* Remove one abandoned publication staging temp under a liveness fence and an
* identity-bound delete. Returns whether the temp was removed; `false` means it
* was deliberately retained (live/indeterminate publisher, not a single-link
* regular file, still inside the grace window, or an identity change between
* capture and delete). Filesystem faults propagate to the caller's best-effort
* handler.
*/
async function reapAbandonedNotificationStagingTemp(input: {
fs: TelegramDaemonFs;
file: string;
claim: NotificationStagingTempClaim;
now: number;
graceMs: number;
pidAlive: (pid: number) => boolean;
}): Promise<boolean> {
// A live or blocked publisher can hold a staged temp far past any grace
// window; only a provably dead publisher's claim is abandoned. `unknown`
// (throwing probe) fails closed.
if (classifyNotificationStagingPublisher(input.claim, input.pidAlive) !== "dead") return false;
const readEndpointFile = input.fs.readEndpointFile;
// Both seams are optional; without them there is no no-follow capture and no
// identity-bound delete, so retain rather than unlink unfenced.
if (!readEndpointFile || !input.fs.exactUnlink) return false;
// No-follow capture: rejects symlinks, directories, and anything that changes
// while it is read. A reparse point or dangling link is therefore retained.
const endpoint = await readEndpointFile(input.file);
if (!(await isSingleLinkRegularFile(input.fs, input.file))) return false;
// Age from the captured (no-follow) mtime rather than a second path-following
// stat, so the grace decision and the delete bind the same inode. Integer ns
// truncation only ever ages the file, never rejuvenates it.
const age = input.now - Number(endpoint.identity.mtimeNs / 1_000_000n);
if (age < input.graceMs) return false;
// The native verifies dev+ino+size+mtimeNs+sha256 before unlinking, so a
// readdir/capture -> replacement ABA cannot delete the fresh generation.
return await exactUnlinkAcceptedWithRetainedEvidence(
input.fs,
input.file,
endpoint.identity,
`.gjc-delete-notification-staging-temp-${crypto.randomUUID()}.json`,
);
}

/** Reap one retained delete quarantine without following or unlinking a successor. */
async function reapNotificationLeakArtifact(input: {
fs: TelegramDaemonFs;
file: string;
now: number;
graceMs: number;
}): Promise<boolean> {
if (!input.fs.readEndpointFile || !input.fs.exactUnlink) return false;
const endpoint = await input.fs.readEndpointFile(input.file);
if (!(await isSingleLinkRegularFile(input.fs, input.file))) return false;
const age = input.now - Number(endpoint.identity.mtimeNs / 1_000_000n);
if (age < input.graceMs) return false;
return await exactUnlinkAcceptedWithRetainedEvidence(
input.fs,
input.file,
endpoint.identity,
`.gjc-exact-unlink-placeholder-${crypto.randomUUID()}.json`,
);
}

/**
* Reap retained exact-unlink / ownership-transition quarantine files older than
* the grace window from the notifications directory.
* the grace window from the notifications directory, plus publication staging
* temps whose publisher is provably dead.
*/
export async function reapStaleNotificationArtifacts(input: {
settings: Settings;
fs?: TelegramDaemonFs;
now?: () => number;
graceMs?: number;
/** Liveness seam used to prove a staging temp's publisher is dead. */
pidAlive?: (pid: number) => boolean;
}): Promise<{ removed: string[]; skipped: number }> {
const fsImpl = input.fs ?? nodeFs;
const paths = daemonPaths(input.settings.getAgentDir());
await ensureDir(fsImpl, paths.dir);
const now = input.now?.() ?? Date.now();
const graceMs = input.graceMs ?? NOTIFICATION_LEAK_ARTIFACT_GRACE_MS;
const pidAlive = input.pidAlive ?? defaultPidAlive;
const removed: string[] = [];
let skipped = 0;
let names: string[];
Expand All @@ -1433,17 +1586,25 @@ export async function reapStaleNotificationArtifacts(input: {
throw error;
}
for (const name of names) {
if (!isNotificationLeakArtifactName(name)) continue;
const stagingTemp = NOTIFICATION_STAGING_TEMP_PATTERN.test(name);
if (!isNotificationLeakArtifactName(name) && !stagingTemp) continue;
const file = path.join(paths.dir, name);
try {
const stat = fsImpl.stat ? await fsImpl.stat(file) : undefined;
const age = stat ? now - stat.mtimeMs : Number.POSITIVE_INFINITY;
if (Number.isFinite(age) && age < graceMs) {
skipped += 1;
if (stagingTemp) {
const claim = parseNotificationStagingTemp(name);
// A staging-temp shape whose PID/timestamp will not parse carries no
// provable claim, so it is retained.
if (!claim) {
skipped += 1;
continue;
}
if (await reapAbandonedNotificationStagingTemp({ fs: fsImpl, file, claim, now, graceMs, pidAlive }))
removed.push(file);
else skipped += 1;
continue;
}
await fsImpl.unlink(file);
removed.push(file);
if (await reapNotificationLeakArtifact({ fs: fsImpl, file, now, graceMs })) removed.push(file);
else skipped += 1;
} catch (error) {
if (isPermanentMissingPathError(error)) continue;
// Best-effort: a busy file must not fail daemon ownership.
Expand All @@ -1462,6 +1623,8 @@ export async function healTelegramDaemonNotificationState(input: {
fs?: TelegramDaemonFs;
now?: () => number;
graceMs?: number;
/** Liveness seam used to prove a staging temp's publisher is dead. */
pidAlive?: (pid: number) => boolean;
}): Promise<{ prunedRoots: string[]; removedArtifacts: string[] }> {
const prune = await pruneMissingNotificationRoots(input);
const reap = await reapStaleNotificationArtifacts(input);
Expand Down Expand Up @@ -5263,6 +5426,7 @@ export class TelegramNotificationDaemon {
settings: this.opts.settings,
fs: this.fsImpl,
now: this.opts.now,
pidAlive: this.opts.pidAlive,
});
} catch (error) {
logger.warn(`notifications: leak-artifact reap failed: ${sanitizeDiagnostic(String(error))}`);
Expand Down
Loading
Loading