fix: delete partial SESSION_LOGS objects when a put fails

[SebTardif]

What Problem This Solves

Fixes an issue where operators who archive interactive session logs to SESSION_LOGS would leave untracked R2 objects behind when one of the three archive puts failed. Session event append, attach, metadata, stop, and terminal finalize all call archiveInteractiveSessionLogs. That function writes events, transcript, and summary under a unique crypto.randomUUID() prefix, then inserts the archive row. If a later put fails, the earlier objects stay in the bucket with no database row. A retry uses a new prefix, so those orphans are never reclaimed.

Why This Change Was Made

Wait for every archive put to finish, then delete the attempted events, transcript, and summary keys before rethrowing the original put error. Existing cleanupSessionLogArchiveObjects already knows how to delete that key triple. The database insert still happens only after all three puts succeed.

User Impact

A failed session-log archive attempt no longer leaves untracked SESSION_LOGS objects. Operators keep the original put error, and a later successful archive still writes a fresh unique prefix. Successful archives are unchanged.

Evidence

Before this change, a SESSION_LOGS bucket that accepted events and transcript then rejected summary left two objects and deleted nothing:

$ node --experimental-strip-types /tmp/crabfleet-F004-proof.mjs
BEFORE error: R2 put failed
BEFORE leftover keys: [
  'orgs/openclaw/interactive-sessions/IS-1/old/events.ndjson',
  'orgs/openclaw/interactive-sessions/IS-1/old/transcript.md'
]
BEFORE deletes: []

After the patch, the same failing summary put still raises R2 put failed, but the production archive path deletes all three attempted keys and leaves an empty store:

AFTER error: R2 put failed
AFTER leftover keys: []
AFTER deletes: [
  'orgs/openclaw/interactive-sessions/IS-1/00000001-0000000000050-200-e2d1ea55-d796-451c-bd48-597c1fdfdf0c/events.ndjson',
  'orgs/openclaw/interactive-sessions/IS-1/00000001-0000000000050-200-e2d1ea55-d796-451c-bd48-597c1fdfdf0c/transcript.md',
  'orgs/openclaw/interactive-sessions/IS-1/00000001-0000000000050-200-e2d1ea55-d796-451c-bd48-597c1fdfdf0c/summary.json'
]

Real behavior proof

• Behavior or issue addressed: A later SESSION_LOGS put failure left earlier unique-key archive objects with no database row and no cleanup.

• Real environment tested: macOS Darwin 25.6.0 arm64, Node v26.7.0, crabfleet checkout /tmp/oc-pr-crabfleet-F004 at branch fix/f004-session-logs-cleanup on top of bdd5083.

• Exact steps or command run after this patch:

  node --experimental-strip-types /tmp/crabfleet-F004-proof.mjs

• Evidence after fix: terminal output from the patched archive path above. The failing summary put still surfaces R2 put failed. The leftover store is empty, and delete ran for events, transcript, and summary under the same unique attempt prefix.

• Observed result after fix: archiveInteractiveSessionLogs no longer keeps untracked SESSION_LOGS objects after a partial put failure. The original put error is still thrown, and no archive row is inserted.

• What was not tested: A live Cloudflare R2 bucket and a production worker process with real session traffic.

Summary

The leak has been present since df9bdc99 in #44 (2026-06-15, 75 days). Event content later changed in #76, but the three-way Promise.all put still had no failure cleanup.

Related: cleanupSessionLogArchiveObjects already deletes obsolete keys after a successful commit. This change uses that helper when the put set fails, after Promise.allSettled so an in-flight sibling put cannot land after cleanup starts.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

[clawsweeper]

Codex review: needs real behavior proof before merge. Reviewed August 31, 2026, 1:32 AM ET / 05:32 UTC.

ClawSweeper review

What this changes

This PR waits for all three interactive-session R2 archive uploads, deletes the attempted objects after any failure, and adds regression coverage for a failed archive write.

Regression provenance

Possible regression — suspected (reviewed change). No predecessor PR is attributed.

Merge readiness

⛔ Blocked until real behavior proof from a real setup is added - 6 items remain

Keep open: the cleanup approach addresses the orphan-object leak, but the branch changes failure selection from the earliest rejected R2 upload to the first upload position, and its supplied proof is mock-only.

Priority: P2
Reviewed head: 76a93be044600dd566bc9e97944d4518e6703f93

Review scores

Measure	Result	What it means
Overall readiness	🦪 silver shellfish (2/6)	The patch is narrowly scoped and test-backed, but it retains a concrete error-order defect and lacks real R2-boundary proof.
Proof confidence	🦪 silver shellfish (2/6)	Needs real behavior proof before merge: The changed production owner is archiveInteractiveSessionLogs, and the supplied terminal trace invokes it after the patch, but the R2 side is a deterministic in-memory store rather than a configured Worker SESSION_LOGS/R2 binding; it is useful supplemental mock proof only. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality	🦐 gold shrimp (3/6)	1 actionable review finding remain.

Verification

Check	Result	Evidence
Real behavior	Needs proof	Needs real behavior proof before merge: The changed production owner is archiveInteractiveSessionLogs, and the supplied terminal trace invokes it after the patch, but the R2 side is a deterministic in-memory store rather than a configured Worker SESSION_LOGS/R2 binding; it is useful supplemental mock proof only. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed	6 items	Introduced cleanup path: The PR replaces fail-fast aggregation with all-settled aggregation, cleans up the three attempted keys on failure, and then rethrows a rejected upload error. Introduced error-order regression: Promise.allSettled preserves input order; results.find therefore selects the events upload failure whenever it failed, even if transcript or summary rejected first. The former Promise.all behavior rejected with the earliest settled failure. Regression test does not cover competing failures: The new test rejects only summary.json, the third input, so it cannot detect an earlier summary/transcript rejection being replaced by a later events failure.
Findings	1 actionable finding	[P2] Preserve the earliest rejected R2 upload
Security	None	None.

How this fits together

The Worker periodically serializes interactive-session events into three R2 snapshot objects and stores their committed pointers in D1. Session lifecycle operations invoke this archive path, so a failed upload must leave neither a usable D1 pointer nor untracked objects.

flowchart LR
  A[Session lifecycle event] --> B[Archive session snapshot]
  B --> C[Three R2 uploads]
  C --> D{All uploads succeed?}
  D -->|Yes| E[Store D1 archive pointers]
  D -->|No| F[Delete attempted R2 objects]
  F --> G[Return upload error]

Loading

Before merge

• Add real behavior proof - Needs real behavior proof before merge: The changed production owner is archiveInteractiveSessionLogs, and the supplied terminal trace invokes it after the patch, but the R2 side is a deterministic in-memory store rather than a configured Worker SESSION_LOGS/R2 binding; it is useful supplemental mock proof only. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
• Preserve the earliest rejected R2 upload (P2) - This repeats the prior P2 blocker. Promise.allSettled reports results in input order, so find can throw a later events-upload failure even when summary or transcript failed first; the prior Promise.all surfaced the earliest rejection. Capture settlement-order failure before cleanup and add a reverse-order failure test.
• Resolve merge risk (P1) - When more than one upload fails, callers can receive a later failure based on upload-array position rather than the first failure they would previously have observed.
• Resolve merge risk (P1) - The supplied trace does not prove cleanup through a configured Worker SESSION_LOGS/R2 binding.
• Resolve merge risk (P1) - Repository policy requires pnpm check, pnpm test, and pnpm macos:test before landing; this read-only review did not rerun them.
• Complete next step (P2) - A focused code-and-test repair can preserve fail-fast error selection; configured R2 proof remains a contributor-owned merge requirement.

Findings

• [P2] Preserve the earliest rejected R2 upload — src/worker/session-log-archive.ts:67-68

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Changed surface	3 files; 99 added, 1 removed	The production change is narrow, while most added lines build focused archive-path test scaffolding.
Production versus tests	production +8/-1; tests +90	The branch adds substantial focused coverage, but that coverage presently omits concurrent failure ordering.

Merge-risk options

Maintainer options:

1. Preserve actual failure order and prove the binding path (recommended)
   Capture the earliest rejected upload before waiting for cleanup, add a competing-failure regression test, and post redacted configured Worker/R2 evidence.
2. Pause for stronger validation
   Keep the PR open until its error semantics and real R2 binding behavior can be verified without changing existing operator diagnostics.

Technical review

Best possible solution:

Preserve the earliest upload rejection while still awaiting all uploads before cleanup, cover competing failures, and provide redacted configured Worker/R2 boundary evidence before merge.

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

Yes for the error-order defect: two deferred upload promises can reject in reverse input order, and the current source will rethrow the lower-index failure. No configured Worker/R2 reproduction was supplied for the cleanup behavior.

Is this the best way to solve the issue?

No: waiting for all uploads before cleanup is appropriate, but selecting the rejection from the all-settled result array does not preserve prior fail-fast error semantics.

Full review comments:

• [P2] Preserve the earliest rejected R2 upload — src/worker/session-log-archive.ts:67-68
  This repeats the prior P2 blocker. Promise.allSettled reports results in input order, so find can throw a later events-upload failure even when summary or transcript failed first; the prior Promise.all surfaced the earliest rejection. Capture settlement-order failure before cleanup and add a reverse-order failure test.
  Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: found and applied where relevant.

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

Labels

Label justifications:

• P2: The PR fixes a bounded archive leak but introduces a normal-priority compatibility regression in the error surfaced to callers.
• merge-risk: 🚨 compatibility: Replacing fail-fast Promise.all with input-order result selection can change which R2 failure existing callers observe.
• rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
• status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The changed production owner is archiveInteractiveSessionLogs, and the supplied terminal trace invokes it after the patch, but the R2 side is a deterministic in-memory store rather than a configured Worker SESSION_LOGS/R2 binding; it is useful supplemental mock proof only. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

Acceptance criteria:

• [P1] Add a competing-rejection test in tests/session-log-archive.test.ts that verifies earliest rejection is rethrown after cleanup.
• [P1] pnpm check.
• [P1] pnpm test.
• [P1] pnpm macos:test.

What I checked:

• Introduced cleanup path: The PR replaces fail-fast aggregation with all-settled aggregation, cleans up the three attempted keys on failure, and then rethrows a rejected upload error. (src/worker/session-log-archive.ts:54, 76a93be04460)
• Introduced error-order regression: Promise.allSettled preserves input order; results.find therefore selects the events upload failure whenever it failed, even if transcript or summary rejected first. The former Promise.all behavior rejected with the earliest settled failure. (src/worker/session-log-archive.ts:67, 76a93be04460)
• Regression test does not cover competing failures: The new test rejects only summary.json, the third input, so it cannot detect an earlier summary/transcript rejection being replaced by a later events failure. (tests/session-log-archive.test.ts:183, 76a93be04460)
• Archive contract: Current documentation defines SESSION_LOGS as three R2 snapshot objects with D1 retaining committed archive pointers, making failed-upload cleanup a valid bounded repair. (docs/runs.md:105, 76a93be04460)
• History and routing: Blame attributes the new aggregation and rethrow lines to this PR; the existing upload lines are a boundary-marked older revision, so their original introduction cannot be established from the locally available parent content. (src/worker/session-log-archive.ts:54, 76a93be04460)
• Provided behavior evidence: The PR body records an after-fix terminal run through the archive owner, but expressly uses a deterministic in-memory R2-like store and does not exercise a configured Worker SESSION_LOGS/R2 binding. (76a93be04460)

Likely related people:

• Vincent Koc: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
• steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

• Capture and rethrow the earliest upload rejection, with a competing-failure regression test.
• Post redacted after-fix evidence from a configured Worker SESSION_LOGS/R2 path; update the PR body to trigger re-review, or ask a maintainer to comment @clawsweeper re-review if needed.

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

• 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.

History

Review history (8 earlier review cycles)

• reviewed 2026-08-30T00:05:20.947Z sha 76a93be :: needs real behavior proof before merge. :: none
• reviewed 2026-08-30T04:08:19.480Z sha 76a93be :: needs real behavior proof before merge. :: none
• reviewed 2026-08-30T08:37:58.162Z sha 76a93be :: needs real behavior proof before merge. :: [P2] Preserve the first observed put failure
• reviewed 2026-08-30T13:02:11.441Z sha 76a93be :: needs real behavior proof before merge. :: [P2] Rethrow the first observed put failure
• reviewed 2026-08-30T16:52:23.475Z sha 76a93be :: needs real behavior proof before merge. :: [P2] Preserve the first settled R2 write failure
• reviewed 2026-08-30T21:56:22.633Z sha 76a93be :: needs real behavior proof before merge. :: [P2] Preserve the earliest upload failure
• reviewed 2026-08-30T23:59:24.589Z sha 76a93be :: needs real behavior proof before merge. :: [P2] Rethrow the earliest failed upload
• reviewed 2026-08-31T03:14:39.019Z sha 76a93be :: needs real behavior proof before merge. :: [P2] Preserve the first-upload failure