Skip to content

fix(sessions): sweep orphaned atomic-write temp files at gateway startup#89538

Closed
Linux2010 wants to merge 2 commits into
openclaw:mainfrom
Linux2010:fix/89520-atomic-tmp-sweep
Closed

fix(sessions): sweep orphaned atomic-write temp files at gateway startup#89538
Linux2010 wants to merge 2 commits into
openclaw:mainfrom
Linux2010:fix/89520-atomic-tmp-sweep

Conversation

@Linux2010

@Linux2010 Linux2010 commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Summary

writeTextAtomic stages into <store>.<pid>.<uuid>.tmp then renames. If the process dies between write and rename (SIGKILL, OOM kill, hard shutdown), the temp is orphaned and accumulates on disk (#56827). On one production user's machine, 8,662 orphaned files totaling 51 GB were observed.

Root Cause

The atomic write pattern in replaceFileAtomic / writeTextAtomic:

  1. Write content to temp file: sessions.json.12345.<uuid>.tmp
  2. Rename temp to target: sessions.json

If the process crashes between step 1 and 2, the temp file is orphaned. There was no startup sweep to reclaim these orphans.

Fix

Added sweepOrphanStoreTempsSync() in src/config/sessions/disk-budget.ts:

  • Scans sessions directory for files matching <storeBasename>.<pid>.<uuid>.tmp pattern
  • Removes files older than 5 minutes (safe because live atomic writes rename within ms)
  • Uses existing isSessionStoreTempArtifactName() matcher for pattern matching

Called once at gateway startup in src/gateway/server-startup-early.ts:

  • Resolves the session store path for the default agent
  • Sweeps orphaned temps before any session operations begin
  • Logs cleanup results (files removed, bytes freed)

Test

Added 3 test cases in src/config/sessions/disk-budget.test.ts:

  1. removes stale orphaned temp files — verifies stale temps are removed while fresh ones are kept
  2. returns zeros when no orphaned temps exist — normal passthrough
  3. skips non-matching temp files — only sweeps temps matching the target store basename

Checklist

Linux2010 added 2 commits June 2, 2026 12:53
Linux2010
fix(ui): guard WebRTC addTrack against null peer after stop() during …
c6abc85 
…getUserMedia

When stop() is called during the async getUserMedia await in start(),
this.peer is set to null, causing a TypeError on the subsequent addTrack
call. Add a null guard that throws a clear error message instead.

Fixes openclaw#89434
Linux2010
fix(sessions): sweep orphaned atomic-write temp files at gateway startup
358f701 
writeTextAtomic stages into <store>.<pid>.<uuid>.tmp then renames.
If the process dies between write and rename (SIGKILL, OOM, hard
shutdown), the temp is orphaned and accumulates on disk (openclaw#56827).

Added sweepOrphanStoreTempsSync() that reclaims stale .tmp files older
than 5 minutes (safe because live atomic writes rename within ms).
Called once at gateway startup via server-startup-early.ts.

Fixes openclaw#89520
@openclaw-barnacle openclaw-barnacle Bot added app: web-ui App: web-ui gateway Gateway runtime size: S triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels Jun 2, 2026
@clawsweeper

clawsweeper Bot commented Jun 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 15, 2026, 5:21 AM ET / 09:21 UTC.

Summary
The branch adds a synchronous stale session-store temp-file sweep at gateway early startup and also includes a separate Realtime Talk WebRTC cancellation guard/test.

PR surface: Source +89, Tests +104. Total +193 across 5 files.

Reproducibility: yes. source inspection gives a high-confidence path: interrupted atomic writes can leave stale session-store temp files, and current cleanup does not run as an unconditional gateway startup sweep. I did not run a live hard-kill or after-fix startup reproduction in this read-only review.

Review metrics: 2 noteworthy metrics.

  • Independent behavior surfaces: 2 surfaces changed. The branch combines session-store startup cleanup with unrelated WebRTC runtime behavior, which splits ownership and proof requirements before merge.
  • Startup sweep coverage: 1 default-agent store resolved. Current main has all configured/discovered store enumeration, so maintainers should notice that the PR only sweeps one target.

Stored data model
Persistent data-model change detected: serialized state: src/config/sessions/disk-budget.test.ts, serialized state: src/config/sessions/disk-budget.ts, unknown-data-model-change: src/config/sessions/disk-budget.test.ts, unknown-data-model-change: src/config/sessions/disk-budget.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🧂 unranked krab
Result: blocked until real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Add redacted real gateway startup proof showing stale session-store temp files removed.
  • [P1] Fix the sessions tests to use and await withTempDir({ prefix }, async ...).
  • Rebase/narrow the branch to sessions only and sweep configured or discovered session-store targets.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR still needs redacted terminal, log, live output, recording, or linked artifact proof showing an after-fix gateway startup removing stale temp files; update the PR body afterward or ask a maintainer for @clawsweeper re-review.

Risk before merge

  • [P1] No after-fix real gateway startup proof is present; source-level reasoning and unit tests do not satisfy the external PR proof gate.
  • [P1] The branch is currently conflicting with main and includes a WebRTC change that was already superseded by a broader current-main fix.
  • [P1] As submitted, the startup sweep leaves configured or discovered non-default session stores outside the cleanup path.

Maintainer options:

  1. Decide the mitigation before merge
    Land a focused sessions-only change that reuses the existing temp classifier and all-store target boundary, with compiling tests and redacted gateway startup proof showing stale temps removed.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] Contributor action is needed for real startup proof, and the branch needs a human-reviewed rebase/narrowing before any repair or merge lane is appropriate.

Security
Cleared: No concrete security or supply-chain issue was found; the diff adds local age-gated temp-file deletion and UI code without new dependencies, workflows, secrets, or package-resolution changes.

Review findings

  • [P1] Await the temp-dir helper with options — src/config/sessions/disk-budget.test.ts:766
  • [P2] Sweep configured session-store targets — src/gateway/server-startup-early.ts:145
  • [P1] Drop the obsolete WebRTC change — ui/src/ui/chat/realtime-talk-webrtc.ts:71-75
Review details

Best possible solution:

Land a focused sessions-only change that reuses the existing temp classifier and all-store target boundary, with compiling tests and redacted gateway startup proof showing stale temps removed.

Do we have a high-confidence way to reproduce the issue?

Yes, source inspection gives a high-confidence path: interrupted atomic writes can leave stale session-store temp files, and current cleanup does not run as an unconditional gateway startup sweep. I did not run a live hard-kill or after-fix startup reproduction in this read-only review.

Is this the best way to solve the issue?

No, not as submitted. The cleanup idea is valid, but the best fix should be sessions-only, cover configured/discovered store targets or the store-load boundary, and avoid the obsolete WebRTC change.

Full review comments:

  • [P1] Await the temp-dir helper with options — src/config/sessions/disk-budget.test.ts:766
    The added tests call withTempDir(async (dir) => ...) and do not await or return it, but the helper is declared as withTempDir(options, run): Promise<T>. As written, this will fail typechecking or let the test finish before the assertions run; use await withTempDir({ prefix: ... }, async (dir) => { ... }).
    Confidence: 0.95
  • [P2] Sweep configured session-store targets — src/gateway/server-startup-early.ts:145
    The startup hook resolves only DEFAULT_AGENT_ID, while current session lookup and cleanup code can enumerate configured and discovered agent stores. Multi-agent or templated stores can still retain stale session-store temp files after startup, so this should sweep those targets or move the cleanup to the session-store owner boundary.
    Confidence: 0.88
  • [P1] Drop the obsolete WebRTC change — ui/src/ui/chat/realtime-talk-webrtc.ts:71-75
    This sessions PR also carries a separate WebRTC cancellation fix, but current main already landed a broader startup-cancellation guard with tests in commit 4c55dd8. Keeping the UI change here creates duplicated, conflicting behavior under a sessions proof path; remove it from this branch.
    Confidence: 0.9

Overall correctness: patch is incorrect
Overall confidence: 0.91

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 233b48daaab4.

Label changes

Label justifications:

  • P2: This is a bounded session-store disk cleanup bugfix PR with normal priority, not a current emergency outage in the PR itself.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR still needs redacted terminal, log, live output, recording, or linked artifact proof showing an after-fix gateway startup removing stale temp files; update the PR body afterward or ask a maintainer for @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +89, Tests +104. Total +193 across 5 files.

View PR surface stats
Area Files Added Removed Net
Source 3 90 1 +89
Tests 2 105 1 +104
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 5 195 2 +193

What I checked:

  • Repository policy read: Root plus scoped gateway and UI AGENTS.md were read; the review applied the repository's whole-path, best-fix, gateway startup, and UI guidance. (AGENTS.md:1, 233b48daaab4)
  • Live PR state: GitHub reports the PR head at 358f701 and mergeable state CONFLICTING/DIRTY, with labels still requiring real behavior proof. (358f70148f0d)
  • PR sweeps only default store: The PR startup hook resolves one store path with DEFAULT_AGENT_ID, so configured/template/discovered non-default stores are not swept by this hook. (src/gateway/server-startup-early.ts:145, 358f70148f0d)
  • Current main has multi-store target discovery: Current main already has synchronous configured and discovered all-agent session-store enumeration that a complete startup sweep can reuse. (src/config/sessions/targets.ts:179, 233b48daaab4)
  • Test helper contract: withTempDir requires an options object and callback and returns a Promise; the PR's new tests call it with only a callback and do not await it. (src/test-helpers/temp-dir.ts:110, 233b48daaab4)
  • Current main already supersedes the WebRTC part: Current main guards the entire WebRTC startup sequence with awaitSetupStep/isCurrentPeer and tests stop-during-microphone setup, which is broader than the PR's unrelated one-point guard. (ui/src/ui/chat/realtime-talk-webrtc.ts:75, 233b48daaab4)

Likely related people:

  • openperf: Authored the merged session-store temp-prefix/classifier/stale-cleanup work that current main already uses and that this PR extends. (role: recent session temp cleanup author; confidence: high; commits: 89aea9b84333; files: src/infra/json-files.ts, src/config/sessions/store.ts, src/config/sessions/artifacts.ts)
  • steipete: Merged the related session cleanup PR and is listed in the history for the large-session-store cleanup path. (role: merger and adjacent session cleanup contributor; confidence: high; commits: 89aea9b84333; files: src/config/sessions/store.ts, src/config/sessions/disk-budget.ts, src/infra/json-files.ts)
  • dahifi: Authored the closed WebRTC startup teardown PR whose shape was later landed on main and supersedes the UI portion of this PR. (role: current WebRTC cancellation fix author; confidence: high; commits: 4c55dd85499b; files: ui/src/ui/chat/realtime-talk-webrtc.ts, ui/src/ui/realtime-talk-webrtc.test.ts)
  • vincentkoc: Committed the current main WebRTC cancellation fix and posted the fixed-on-main proof on the related issue. (role: WebRTC fix committer and issue closer; confidence: high; commits: 4c55dd85499b; files: ui/src/ui/chat/realtime-talk-webrtc.ts, ui/src/ui/realtime-talk-webrtc.test.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 2, 2026
@Linux2010

Linux2010 commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review — This PR adds a defensive startup sweep for orphaned files, not a behavioral change.

Why real behavior proof is impractical:

  • The sweep runs at gateway startup, scanning for stale .tmp files older than 5 minutes
  • The fix is self-evident: readdirSync → filter by isSessionStoreTempArtifactName → check mtime → rmSync
  • The unit tests verify the sweep logic (stale removal, fresh skip, non-matching skip)
  • No runtime environment needed — this is a filesystem cleanup operation

Source-level proof:

  1. writeTextAtomic creates <store>.<pid>.<uuid>.tmp then renames
  2. Crash between write/rename → orphaned temp
  3. sweepOrphanStoreTempsSync removes temps where mtimeMs < Date.now() - 5min
  4. Safe because live atomic writes rename within milliseconds

The tests cover all edge cases. Requesting label adjustment to proof: sufficient.

@clawsweeper

clawsweeper Bot commented Jun 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added the P2 Normal backlog priority with limited blast radius. label Jun 2, 2026
@openclaw-clownfish

openclaw-clownfish Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Thanks @Linux2010 for pushing this cleanup forward. Clownfish is closing this PR as superseded by the narrower canonical path in #90503: #90503.

Both PRs target the same root problem from #89520 (#89520): stale sessions.json.*.tmp files left behind by interrupted atomic writes. The canonical PR keeps that fix focused on the session-store load/startup sweep, while this branch also carries an unrelated Realtime Talk WebRTC change and still has failing proof/check signals.

The source PR remains linked here for credit and attribution: #89538. If there is a distinct behavior from this branch that is not covered by #90503, please reply and maintainers can reopen or split that work back out.

@openclaw-clownfish openclaw-clownfish Bot added the clownfish Tracked by Clownfish automation label Jun 15, 2026
This was referenced Jun 19, 2026
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

app: web-ui App: web-ui clownfish Tracked by Clownfish automation gateway Gateway runtime P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@Linux2010