[codex] Prevent background Git fetches from leaking temp packs#3537

Deletes unrelated fetch temp packs

Medium Severity

After a failed status fetchRemoteForStatus, cleanup removes every new tmp_pack_* name versus a pre-fetch snapshot, not packs created only by that fetch. Another git fetch (for example fetchRemote) can fail, release FETCH_HEAD.lock, and leave temp packs that this path then deletes.

^{Reviewed by Cursor Bugbot for commit 58340604f0b7967e7a5a3e532993f3443442d14e. Configure here.}

🟡 Medium telemetry/DesktopTelemetryPublisher.ts:234

A telemetry consumer connecting via the encoded stream can miss the current snapshot. snapshots first reads Ref.get(latest) into a one-element stream, then subscribes to Stream.fromPubSub(changes). A snapshot published between the Ref.get(latest) read and the Stream.fromPubSub(changes) subscription is absent from both sources, so the consumer stays stale until the next periodic sample (up to 30 seconds in background mode). Subscribe to changes before reading latest, or use a replay-capable source.

🟡 Medium resourceTelemetry/NativeTelemetryClient.ts:369

Successful snapshot and historyChunk events update state.status to "healthy" and clear lastError but never call publishHealth, so the healthChanges stream is not notified. After a recoverable monitor error sets the status to "degraded" and publishes that transition, a later successful snapshot restores the internal state to "healthy" without emitting a health change — subscribers that listen on healthChanges (e.g. ResourceTelemetry) never see the recovery and can remain visibly degraded indefinitely. The hello and error branches both call publishHealth after their Ref.update, but the snapshot and historyChunk branches omit it. Add a publishHealth call after each of those state updates.

🟡 Medium background/BackgroundPolicy.ts:45

reportClientActivity always uses DEFAULT_LEASE_TTL_MS (or the client-supplied ttlMs) and never reads ResolvedBackgroundActivitySettings.idleClientTtl, so changing the server's idleClientTtl override has no effect on lease expiry. Clients keep getting 45-second leases regardless of the configured value, making the idleClientTtl background-activity setting non-functional. Consider clamping the resolved idleClientTtl setting instead of the hard-coded constant when input.ttlMs is not provided.

🟡 Medium resourceTelemetry/NativeTelemetryClient.ts:652

setHostPowerState and changeLiveSubscriberCount both atomically update collectionControl and then send commands to the sidecar, but the read-modify-send sequence is not protected across the two operations. When they run concurrently, the one whose Ref.modify executes first can have its commands delivered second, so the sidecar ends up with stale settings. For example, if a subscriber arrives while a host-power update is in flight, the power update's stale sampleIntervalMs/setStreaming commands can overwrite the subscriber's newer commands, leaving the sidecar at the wrong interval or with streaming disabled despite an active subscriber. Consider serializing the full read-modify-send sequence (e.g., with a dedicated mutex around both the Ref.modify and the applyCollectionControl call) so command delivery order always matches control-update order.

🟠 High Layers/EventNdjsonLogger.ts:226

enforceRetention deletes every file in the store's directory whose name matches /\.log(?:\.\d+)?$/u — including unrelated log files from other components. Since makeEventNdjsonLogStore accepts arbitrary paths (e.g. adapter-supplied nativeEventLogPath), pointing it at a shared log directory causes the initial and periodic retention sweeps to permanently delete other components' .log files once they exceed maxAgeMs or contribute to maxTotalBytes. The PROVIDER_LOG_FILE_PATTERN regex is too broad to scope deletion to this store's own thread-log files. Consider restricting candidates to the thread-segment naming scheme (e.g. ^[A-Za-z0-9_-]+\.log(?:\.\d+)?$ matching toSafeThreadAttachmentSegment output) or requiring a dedicated directory.

🟡 Medium resourceTelemetry/DesktopTelemetryReceiver.ts:356

The stale-check loop reads latest, marks the snapshot stale, then unconditionally calls Ref.set(latest, ...). If a fresh telemetry message arrives between the read and the set, the checker overwrites the newer snapshot with the older stale-marked one and publishes that regressed data on changes. Consider using Ref.modify with a timestamp comparison so a newer sample is never replaced by an older stale-marked snapshot.

🟡 Medium settings/SettingsPanels.tsx:223

backgroundActivityOverrideSettings builds the custom configuration only from current.overrides, but resolveServerBackgroundActivitySettings can display values sourced from legacy flat fields (automaticGitFetchInterval, providerHealthRefreshInterval) when current.backgroundActivity is still default with empty overrides. Editing a single interval control then writes a custom backgroundActivity containing only that one override, so the other displayed legacy interval silently reverts to the base-profile preset — e.g. changing provider health loses a user's legacy custom Git-fetch interval. Build the patch from the effective resolved settings (or migrate legacy overrides before applying the patch) so all currently displayed values are preserved.

🟡 Medium Layers/EventNdjsonLogger.ts:368

When a sink.write call throws, drainPending adds the thread segment to failedSegments permanently. Every subsequent drain skips that segment at the if (failedSegments.has(threadSegment)) continue check, but still clears all pending records — so a transient write error (temporary disk pressure, short-lived FS error) permanently disables logging for that thread for the remaining store lifetime with no retry or recovery, and silently drops every future record for that thread. Previously, batch write failures did not mark the thread writer permanently failed. Consider removing the failedSegments add on write failure (or retrying on the next drain) so transient errors don't permanently block the segment.

🟡 Medium app/DesktopBackendOutputLog.ts:440

persistFailure clears the buffered chunks from sessionRef before writing them to the log file. Because writeBackendChildLogRecord catches encoding and write failures and returns success, a transient filesystem error causes the buffered backend output to be permanently discarded — a retry cannot recover it, and the next beginSession overwrites the session. Clear the buffer only after the writes have succeeded, or propagate failures so the session data is retained on error.

🟡 Medium resourceTelemetry/ResourceTelemetryHistory.ts:152

buildResourceTelemetryHistory attributes every snapshot in the window using only the current desktopSnapshot.electronPid as electronRootPids. When Electron restarts mid-window, samples from the prior Electron PID and its descendants are mislabeled as server-child, and any unrelated process that happened to reuse the current PID is mislabeled as Electron. This corrupts the per-group buckets and topProcesses.category values for any history window spanning a restart. The fix requires per-snapshot Electron identity data or reliable per-sample inference rather than applying one PID set to all historical samples.

🟡 Medium

https://github.com/pingdotgg/t3code/blob/58340604f0b7967e7a5a3e532993f3443442d14e/apps/server/src/diagnostics/ProcessDiagnostics.ts#L1

The read function filters only the server PID out of snapshot.processes, so independent Electron roots and the resource-monitor process are returned as server child processes. This inflates processCount, totalRssBytes, and totalCpuPercent, and their depth is clamped to 0 via Math.max(0, entry.depth - 1) even though they are not descendants of the server. The filter at line 78 should restrict the list to the server/backend descendant tree rather than all telemetry processes.

🟡 Medium resourceTelemetry/ResourceTelemetryHistory.ts:149

buildResourceTelemetryHistory filters snapshots only with a lower bound (>= windowStartMs), so snapshots with sampledAtUnixMs > readAtMs pass through. Those snapshots are excluded from every bucket by buildBuckets (which caps each bucket's end at nowMs), but they are still iterated into processSamples and counted in retainedSampleCount, so the returned history has buckets that omit the latest snapshots while topProcesses includes them — and the process "current" values can be later than readAt. Add an upper-bound filter (sampledAtUnixMs <= readAtMs) alongside the existing lower-bound filter.

🟡 Medium resourceTelemetry/Model.ts:396

When an Electron metric arrives without a matching native sample, mergeProcesses inserts a synthetic process keyed by metric.creationTimeMs at line 387. A subsequent native sample for the same PID that passes the 2-second tolerance check (line 391-395) is then keyed by its own different startTimeMs. Because processIdentityKey (line 85-87) requires exact equality, the same live process is recorded as both an exit (old identity) and a start (new identity), which resets firstSeenAt/deltas and inflates the processStarts/processExits lifecycle counters. This happens whenever desktop telemetry arrives before the native monitor first observes the Electron process.

🟡 Medium background/BackgroundPolicy.ts:238

make never subscribes to serverSettings.streamChanges, so changing background-activity settings (e.g. enabling a battery pause while the host is already on battery) does not produce a new BackgroundPolicySnapshot on streamChanges until an unrelated host-power, lease, or 15-second cleanup event occurs. Stream consumers continue using the old policy for up to 15 seconds. Subscribe to settings changes and invoke publishSnapshot, as is done for host-power changes.

🟡 Medium resourceTelemetry/Model.ts:456

mergeProcesses classifies a process as electron-main solely because its PID is in electronRootPids, without validating the process start time against the desktop metric's creationTimeMs. When a stale desktop snapshot is replayed and the PID has been reused by an unrelated process (e.g. a server child), that process and its descendants are misattributed to Electron. The same stale PID is correctly rejected by metricsByPid via the ELECTRON_IDENTITY_TOLERANCE_MS check, but the explicitElectronRootPids.has(process.pid) branch bypasses that validation. Consider keying electronRootPids by process identity (PID + start time) or validating the start time before classifying a root as electron-main.