Skip to content

Commit 8428a0f

Browse files
authored
fix: [sc-106111] Sweep downloaded installer binaries so temp stops filling (#102)
Downloads now land in <data directory>/updates (0700) instead of the shared system temp directory, and SweepStaleInstallers reclaims installer binaries older than 24 hours at service startup - from that directory and from the legacy temp location. Matches the conservative SweepStaleScripts pattern: regular files only, the exact installer-<digits>.bin shape os.CreateTemp produces, best effort throughout. Integration test run 31712035358 green on all three platforms.
1 parent d5ead54 commit 8428a0f

12 files changed

Lines changed: 1018 additions & 32 deletions

File tree

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
name: Installer sweep fixture
2+
description: >
3+
Seed (mode=seed) or verify (mode=assert) the file fixture for the startup
4+
downloaded-installer sweep scenario. See fixture.ps1 for what the fixture
5+
contains and asserts. The updates directory lives under the installation's
6+
data directory and belongs to the service account (root / SYSTEM), so the
7+
shared implementation runs elevated: under sudo on Unix, natively on the
8+
already-elevated Windows runner.
9+
10+
inputs:
11+
mode:
12+
description: "seed to place the fixture files, assert to verify the sweep result"
13+
required: true
14+
updates_dir:
15+
description: "Updates directory the auto-updater downloads installer binaries into"
16+
required: true
17+
stale_age_hours:
18+
description: "How far back to backdate the files that must be swept"
19+
required: false
20+
default: "48"
21+
22+
runs:
23+
using: composite
24+
steps:
25+
- name: Run installer sweep fixture (Unix)
26+
if: runner.os != 'Windows'
27+
shell: bash
28+
env:
29+
MODE: ${{ inputs.mode }}
30+
UPDATES_DIR: ${{ inputs.updates_dir }}
31+
STALE_AGE_HOURS: ${{ inputs.stale_age_hours }}
32+
run: |
33+
set -euo pipefail
34+
# Resolve pwsh up front: sudo resets PATH via secure_path on some runners.
35+
pwsh_path="$(command -v pwsh)"
36+
sudo "$pwsh_path" -NoProfile -File "$GITHUB_ACTION_PATH/fixture.ps1" \
37+
-Mode "$MODE" \
38+
-UpdatesDir "$UPDATES_DIR" \
39+
-StaleAgeHours "$STALE_AGE_HOURS"
40+
41+
- name: Run installer sweep fixture (Windows)
42+
if: runner.os == 'Windows'
43+
shell: pwsh
44+
env:
45+
MODE: ${{ inputs.mode }}
46+
UPDATES_DIR: ${{ inputs.updates_dir }}
47+
STALE_AGE_HOURS: ${{ inputs.stale_age_hours }}
48+
run: |
49+
& "$env:GITHUB_ACTION_PATH/fixture.ps1" `
50+
-Mode $env:MODE `
51+
-UpdatesDir $env:UPDATES_DIR `
52+
-StaleAgeHours ([int]$env:STALE_AGE_HOURS)
Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,115 @@
1+
#!/usr/bin/env pwsh
2+
#Requires -Version 7
3+
4+
<#
5+
.SYNOPSIS
6+
Seeds (mode seed) or verifies (mode assert) the file fixture for the startup
7+
downloaded-installer sweep scenario (sc-106111).
8+
9+
.DESCRIPTION
10+
Three control files are placed in the agent's updates directory:
11+
12+
installer-900000001.bin matches the name pattern the updater downloads under
13+
and is aged past the sweep threshold, so it must be
14+
reclaimed
15+
installer-900000002.bin matches the pattern but keeps the current timestamp,
16+
standing in for the installer of the update that is
17+
still running - the file the sweep must never take
18+
vendor-installer.bin is aged but is not a name the agent ever creates, so
19+
the sweep must leave it alone however stale it is
20+
21+
The files are sized in megabytes rather than bytes, so the reclaimed space is
22+
visible in the directory listing the fixture prints: the leak this scenario
23+
covers is measured in tens of megabytes per update, not in file counts.
24+
25+
The updates directory is created if it does not exist. An installation that has
26+
never completed an update has never created it, which is the normal state on a
27+
CI runner - the scenario exercises the sweep, not the download.
28+
29+
Ageing is the only thing simulated here: it is exactly what a long-lived install
30+
does by staying up. The production 24h threshold stays in play, so no test-only
31+
override is needed in the agent.
32+
33+
Symlink handling is asserted by the unit tests rather than here. Aging a symlink
34+
without following it needs a different call on every platform, and an un-aged
35+
link would survive on the age check alone - making the assertion pass without
36+
proving the sweep declined to follow it.
37+
#>
38+
39+
param(
40+
[Parameter(Mandatory)][ValidateSet('seed', 'assert')][string]$Mode,
41+
[Parameter(Mandatory)][string]$UpdatesDir,
42+
[int]$StaleAgeHours = 48
43+
)
44+
45+
$ErrorActionPreference = 'Stop'
46+
47+
$stale = Join-Path $UpdatesDir 'installer-900000001.bin'
48+
$fresh = Join-Path $UpdatesDir 'installer-900000002.bin'
49+
$foreign = Join-Path $UpdatesDir 'vendor-installer.bin'
50+
51+
function Show-UpdatesDir {
52+
Write-Output "Contents of ${UpdatesDir}:"
53+
Get-ChildItem -LiteralPath $UpdatesDir -Force |
54+
Select-Object Name, Length, LastWriteTime |
55+
Format-Table |
56+
Out-String |
57+
Write-Output
58+
}
59+
60+
if ($Mode -eq 'seed') {
61+
if (-not (Test-Path -LiteralPath $UpdatesDir)) {
62+
New-Item -ItemType Directory -Path $UpdatesDir -Force | Out-Null
63+
Write-Output "Created updates directory $UpdatesDir"
64+
}
65+
66+
# 4 MiB each: small enough to write quickly, large enough that the listing
67+
# shows the sweep reclaiming real space rather than empty files.
68+
$payload = [byte[]]::new(4MB)
69+
foreach ($path in @($stale, $fresh, $foreign)) {
70+
[System.IO.File]::WriteAllBytes($path, $payload)
71+
}
72+
73+
$aged = (Get-Date).AddHours(-$StaleAgeHours)
74+
foreach ($path in @($stale, $foreign)) {
75+
(Get-Item -LiteralPath $path -Force).LastWriteTime = $aged
76+
Write-Output "Aged $path to $aged"
77+
}
78+
79+
Show-UpdatesDir
80+
exit 0
81+
}
82+
83+
if (-not (Test-Path -LiteralPath $UpdatesDir)) {
84+
Write-Error "Updates directory does not exist: $UpdatesDir"
85+
exit 1
86+
}
87+
88+
$failed = $false
89+
90+
if (Test-Path -LiteralPath $stale) {
91+
Write-Output "FAIL: stale installer file was not swept: $stale"
92+
$failed = $true
93+
}
94+
else {
95+
Write-Output "OK: stale installer file was swept: $stale"
96+
}
97+
98+
foreach ($path in @($fresh, $foreign)) {
99+
if (Test-Path -LiteralPath $path) {
100+
Write-Output "OK: file the sweep must not touch survived: $path"
101+
}
102+
else {
103+
Write-Output "FAIL: sweep removed a file it must not touch: $path"
104+
$failed = $true
105+
}
106+
}
107+
108+
Show-UpdatesDir
109+
110+
if ($failed) {
111+
Write-Error 'Startup installer sweep did not behave as expected'
112+
exit 1
113+
}
114+
115+
Write-Output 'Startup installer sweep reclaimed the stale downloaded installer and nothing else'

.github/workflows/integration-test.yml

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1555,6 +1555,78 @@ jobs:
15551555
scripts_dir: ${{ steps.inflight_script.outputs.scripts_dir }}
15561556
orphan_path: ${{ steps.inflight_script.outputs.path }}
15571557

1558+
# ---- Downloaded installer sweep scenario (sc-106111) ----
1559+
# Every auto-update downloads a full agent binary and executes it as the
1560+
# installer. The installer is spawned detached and the process that could
1561+
# delete it afterwards is the one the installer replaces, so the download
1562+
# path cannot clean up after itself and nothing else used to - one
1563+
# orphaned binary accumulated per update for the lifetime of the install,
1564+
# until the volume filled and took future updates and command execution
1565+
# with it. Verify the startup sweep now reclaims them.
1566+
#
1567+
# The fixture is planted rather than produced by a real update cycle: the
1568+
# sweep is what this ticket changed, and the download side (where the file
1569+
# lands, under what name) is covered by unit tests that assert the two
1570+
# halves share one pattern constant. Ageing is the only simulated part, so
1571+
# the production 24h threshold stays in play with no test-only override.
1572+
#
1573+
# Only the org's own updates directory is asserted here. The sweep also
1574+
# covers the legacy shared temp directory that older agents downloaded
1575+
# into, but that is the *service account's* temp directory - notably root's
1576+
# private TMPDIR on macOS, not the runner's - so a fixture planted from
1577+
# the runner would land somewhere the service never reads. That half is
1578+
# covered by TestExecute_SweepsStaleInstallerFilesOnStartup, which runs
1579+
# Execute in-process and can see its own temp directory.
1580+
- name: Seed the installer sweep fixture
1581+
uses: ./.github/actions/installer-sweep-fixture
1582+
with:
1583+
mode: seed
1584+
updates_dir: ${{ matrix.config_dir }}/${{ vars.IT_ORG_ID }}/updates
1585+
1586+
- name: Capture installer sweep log baseline count
1587+
id: installer_sweep_baseline
1588+
shell: pwsh
1589+
env:
1590+
LOG_FILE: ${{ matrix.log_file }}
1591+
run: |
1592+
$content = Get-Content $env:LOG_FILE -Raw -ErrorAction SilentlyContinue
1593+
$swept = if ($content) { ([regex]::Matches($content, [regex]::Escape("Swept stale installer files"))).Count } else { 0 }
1594+
"swept=$swept" >> $env:GITHUB_OUTPUT
1595+
Write-Output "Baseline counts -> swept=$swept"
1596+
1597+
- name: Restart agent to trigger the installer sweep
1598+
uses: ./.github/actions/run-agent
1599+
with:
1600+
binary: ${{ matrix.binary }}
1601+
args: --update --org-id ${{ vars.IT_ORG_ID }} --no-auto-updates ${{ matrix.no_auto_updates_extra }} --github-token ${{ secrets.GITHUB_TOKEN }}
1602+
1603+
- name: Wait for and assert the installer sweep was logged
1604+
shell: pwsh
1605+
env:
1606+
LOG_FILE: ${{ matrix.log_file }}
1607+
BASELINE: ${{ steps.installer_sweep_baseline.outputs.swept }}
1608+
run: |
1609+
$baseline = [int]$env:BASELINE
1610+
for ($i = 1; $i -le 60; $i++) {
1611+
$content = Get-Content $env:LOG_FILE -Raw -ErrorAction SilentlyContinue
1612+
$swept = if ($content) { ([regex]::Matches($content, [regex]::Escape("Swept stale installer files"))).Count } else { 0 }
1613+
if ($swept -gt $baseline) {
1614+
Write-Output "Installer sweep logged (count $swept > baseline $baseline) after ~$($i * 2)s"
1615+
exit 0
1616+
}
1617+
Start-Sleep -Seconds 2
1618+
}
1619+
Write-Output "---- last 40 log lines ----"
1620+
Get-Content $env:LOG_FILE -Tail 40 -ErrorAction SilentlyContinue
1621+
Write-Error "Installer sweep was never logged (count stayed at $baseline)"
1622+
exit 1
1623+
1624+
- name: Assert stale installers swept and live files untouched
1625+
uses: ./.github/actions/installer-sweep-fixture
1626+
with:
1627+
mode: assert
1628+
updates_dir: ${{ matrix.config_dir }}/${{ vars.IT_ORG_ID }}/updates
1629+
15581630
# ---- Capped and jittered auto-update retry scenario (sc-106110) ----
15591631
# The auto-update retry delay was base * (1 << attempt) with no ceiling and
15601632
# no jitter. Unjittered, every agent that failed against the release

CLAUDE.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ Required tools:
6363
previous file byte-identical. See the README's "Waiting for the Old Agent
6464
Process to Exit" section.
6565

66-
- **internal/agent/**: Device configuration, installation paths, and OS-specific host information. The auto-update retry schedule is capped (1 hour, or a quarter of the check interval when shorter) and jittered (±25%) via `utils.JitteredBackoff`, the same helper the postback retry schedule uses, so the doubling cannot overflow into a negative sleep that busy-spins and a fleet-wide release-endpoint outage cannot produce a synchronized retry storm. See the README's "Capped and Jittered Auto-Update Retries" section.
66+
- **internal/agent/**: Device configuration, installation paths, and OS-specific host information. Auto-update installers are downloaded into `<data directory>/updates` (a `0700` directory the agent owns) rather than the shared system temp directory, and `SweepStaleInstallers` reclaims installer binaries older than 24 hours at service startup — from that directory and from the legacy temp location — so the binaries a detached installer necessarily leaves behind stop accumulating one per update. See the README's "Reclaiming Downloaded Installer Binaries" section. The auto-update retry schedule is capped (1 hour, or a quarter of the check interval when shorter) and jittered (±25%) via `utils.JitteredBackoff`, the same helper the postback retry schedule uses, so the doubling cannot overflow into a negative sleep that busy-spins and a fleet-wide release-endpoint outage cannot produce a synchronized retry storm. See the README's "Capped and Jittered Auto-Update Retries" section.
6767
- **internal/interpreter/**: Command execution engine supporting both PowerShell and Bash interpreters
6868
- **internal/mqtt/**: Azure IoT Hub MQTT client implementation with auto-reconnection
6969
- **internal/service/**: Cross-platform service management utilities. On Windows, `Stop()` waits a bounded, documented deadline (5 minutes) for the service to reach `Stopped` and otherwise returns an error naming the service and last observed state, so a wedged service aborts an update/install/uninstall with an actionable message instead of hanging forever. See the README's "Bounded Windows Service Stop" section.

README.md

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -524,6 +524,51 @@ The backoff wait stays interruptible by the service stop signal, so a stop is
524524
never delayed by a pending retry, and a retry that succeeds resets the schedule
525525
for the next cycle.
526526

527+
### Reclaiming Downloaded Installer Binaries
528+
529+
Every auto-update downloads a full agent binary and executes it as the installer.
530+
That file has to survive the download — the installer is spawned detached and the
531+
process that could delete it afterwards is the one the installer replaces — so
532+
the agent cannot clean up after itself on the update path. Nothing else did
533+
either, so one orphaned binary (tens of megabytes) accumulated per update for the
534+
lifetime of the installation. On the space-constrained systems where that matters
535+
most — thin VDI images, small VM system disks, appliances — a full temp volume is
536+
not just an agent problem: it breaks Windows Installer, application logging, and
537+
anything else that needs scratch space, and the agent's own next update fails
538+
because it can no longer allocate a temp file.
539+
540+
Two changes reclaim the space:
541+
542+
- **Downloads land in a directory the agent owns.** Installers are written to
543+
`<data directory>/updates` (`C:\ProgramData\RewstRemoteAgent\<orgId>\updates`,
544+
`/etc/rewst_remote_agent/<orgId>/updates`,
545+
`/Library/Application Support/rewst_remote_agent/<orgId>/updates`) instead of
546+
the shared system temp directory, with the directory created `0700`. A full
547+
agent binary is no longer left executable and world-readable, the sweep below
548+
only ever runs against a directory this agent created, and endpoints that mount
549+
`/tmp` `noexec` — a common hardening baseline — can execute the installer at
550+
all. Uninstall already removes the data directory wholesale, so nothing is left
551+
behind.
552+
- **A startup sweep removes what previous updates left.** On every service start,
553+
after the service has reported itself running, installer binaries older than
554+
**24 hours** are removed. Because a successful update restarts the agent, each
555+
start reclaims the previous update's installer and leaves the current one
556+
alone, so steady-state usage is a single file rather than one per update. The
557+
legacy shared temp directory is swept as well, so an upgraded endpoint reclaims
558+
everything it has accumulated since it was installed rather than only stopping
559+
the growth from here on.
560+
561+
The sweep is deliberately conservative, matching the existing stale-script sweep:
562+
only regular files (never symlinks or device nodes) whose name is exactly the
563+
`installer-<digits>.bin` pattern `os.CreateTemp` produces, and only those past the
564+
age threshold — which is why it is safe to point at a directory shared with the
565+
rest of the system. The pattern is a shared constant used by both the download
566+
and the sweep, so the two cannot drift. It is best effort throughout: an
567+
unreadable directory or an unremovable file (a Windows installer still running
568+
holds its own image open) is logged and skipped, never failing or delaying agent
569+
startup. A non-zero number of removals is logged at `Info` with the count and
570+
directory; individual removals are logged at `Debug`.
571+
527572
### Notification Plugin Supervision
528573

529574
Notification plugins run as separate subprocesses reached over RPC, and every

cmd/agent_smith/service.go

Lines changed: 28 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -91,11 +91,11 @@ func (svc *serviceContext) loadLog() (*os.File, error) {
9191
return logFile, nil
9292
}
9393

94-
// scriptsOrgId returns the org id whose scripts directory the executor writes
95-
// command script files to. The executor derives that path from the device
96-
// config's org id, so the startup sweep must use the same value; svc.OrgId (from
94+
// sweepOrgId returns the org id whose directories the startup sweeps reclaim
95+
// files from. The executor and the updater both derive their paths from the
96+
// device config's org id, so the sweeps must use the same value; svc.OrgId (from
9797
// the command line) is only a fallback for a config that omits it.
98-
func (svc *serviceContext) scriptsOrgId(device agent.Device) string {
98+
func (svc *serviceContext) sweepOrgId(device agent.Device) string {
9999
if device.RewstOrgId != "" {
100100
return device.RewstOrgId
101101
}
@@ -286,11 +286,34 @@ func (svc *serviceContext) Execute(
286286
// The org id is taken from the device config rather than the command line so
287287
// the swept directory is exactly the one the executor writes to.
288288
interpreter.SweepStaleScripts(
289-
agent.GetScriptsDirectory(svc.scriptsOrgId(device)),
289+
agent.GetScriptsDirectory(svc.sweepOrgId(device)),
290290
interpreter.DefaultStaleScriptAge,
291291
logger,
292292
)
293293

294+
// Reclaim the installer binaries the auto-updater downloaded for previous
295+
// updates. Download has to keep the file it created so the installer can be
296+
// executed, and the process that could delete it afterwards is the one the
297+
// installer replaces, so the only safe moment to remove it is a later start —
298+
// by which time an installer that is still running is far younger than the
299+
// age threshold and is left alone. Same placement and best-effort contract as
300+
// the script sweep above.
301+
agent.SweepStaleInstallers(
302+
agent.GetUpdatesDirectory(svc.sweepOrgId(device)),
303+
agent.DefaultStaleInstallerAge,
304+
logger,
305+
)
306+
307+
// Agents released before the download moved into the org's own updates
308+
// directory left their installers in the shared system temp directory, where
309+
// nothing has ever removed them. Sweep that location too so an upgraded
310+
// endpoint reclaims the binaries it has been accumulating since it was
311+
// installed, rather than only stopping the growth from here on. The matcher
312+
// is the same conservative one — this agent's exact temp name pattern,
313+
// regular files only, a day old at minimum — which is what makes it safe to
314+
// point at a directory shared with the rest of the system.
315+
agent.SweepStaleInstallers(os.TempDir(), agent.DefaultStaleInstallerAge, logger)
316+
294317
rg := utils.ReconnectTimeoutGenerator{}
295318

296319
for {

0 commit comments

Comments
 (0)