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 September 1, 2026, 8:06 AM ET / 12:06 UTC.

ClawSweeper review

What this changes

This PR waits for all three interactive-session archive uploads, deletes the attempted R2 keys after any failure, and adds a regression test for a failed summary upload.

Merge readiness

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

Keep open: the cleanup approach is useful, but it still rethrows by upload-array order rather than the earliest failed upload, and the supplied after-fix evidence uses a Map-backed substitute rather than a Worker R2 binding.

Priority: P2
Reviewed head: 76a93be044600dd566bc9e97944d4518e6703f93

Review scores

Measure	Result	What it means
Overall readiness	🦪 silver shellfish (2/6)	The patch is focused and covers cleanup, but it retains a concrete dual-failure error-selection defect and lacks real Worker/R2 behavior proof.
Proof confidence	🦪 silver shellfish (2/6)	Needs real behavior proof before merge: The changed production owner is the Worker archive service, but the supplied terminal trace and added test exercise it with a Map-backed put/delete substitute rather than a configured Worker SESSION_LOGS binding and real R2 client; the after-fix result is therefore supplemental 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 the Worker archive service, but the supplied terminal trace and added test exercise it with a Map-backed put/delete substitute rather than a configured Worker SESSION_LOGS binding and real R2 client; the after-fix result is therefore supplemental 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	5 items	Introduced error selection: The introduced code waits for all uploads, then uses results.find; Promise.allSettled preserves input order, so a later event-upload rejection is selected over an earlier summary-upload rejection. Test coverage gap: The added test has only one rejected upload, so it proves cleanup but cannot detect the dual-rejection ordering error. Pre-PR behavior: The merged session-ledger implementation still used Promise.all with no failure cleanup; the fetched main/base commit therefore does not already contain this fix.
Findings	1 actionable finding	[P2] Rethrow the earliest upload failure
Security	None	None.

How this fits together

The Worker turns interactive-session rows and events from D1 into three optional SESSION_LOGS R2 archive objects, then records their keys in D1. Session lifecycle paths call this archive function, so upload failures must leave no untracked objects and preserve useful operator diagnostics.

flowchart LR
  A[Session lifecycle event] --> B[Worker archive service]
  B --> C[Read D1 session and events]
  C --> D[Write three R2 archive objects]
  D --> E{All uploads succeed?}
  E -->|Yes| F[Store archive keys in D1]
  E -->|No| G[Delete attempted R2 keys]
  G --> H[Return upload error]

Loading

Before merge

• Add real behavior proof - Needs real behavior proof before merge: The changed production owner is the Worker archive service, but the supplied terminal trace and added test exercise it with a Map-backed put/delete substitute rather than a configured Worker SESSION_LOGS binding and real R2 client; the after-fix result is therefore supplemental 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.
• Rethrow the earliest upload failure (P2) - allSettled returns results in input order, not settlement order. If summary.json rejects first and events.ndjson rejects later, this selects and throws the events error, contrary to preserving the original failure. This was already raised for this unchanged head; capture the first rejection as it occurs and add a staged dual-rejection test.
• Resolve merge risk (P1) - If two R2 uploads reject at different times, operators can receive the later failure merely because that upload appears earlier in the input array.
• Resolve merge risk (P1) - The available after-fix trace does not establish cleanup behavior through a configured Worker SESSION_LOGS binding and real R2 client.

Findings

• [P2] Rethrow the earliest upload failure — src/worker/session-log-archive.ts:67-69

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Code and test delta	production +8/-1, tests +90, docs +1	The narrow production change is accompanied by substantial failure-path scaffolding, but the new coverage currently exercises only one rejection.

Merge-risk options

Maintainer options:

1. Decide the mitigation before merge
   Capture the first rejection as uploads settle, retain the all-settled cleanup fence, add a staged dual-failure regression, and provide redacted Worker/R2 after-fix output before merge.
2. Pause or close
   Do not merge this PR until maintainers decide whether the risk is worth taking.

Technical review

Best possible solution:

Capture the first rejection as uploads settle, retain the all-settled cleanup fence, add a staged dual-failure regression, and provide redacted Worker/R2 after-fix output before merge.

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

Yes, source establishes the failure path: two staged rejected upload promises make the introduced array-order selection return the wrong error after cleanup.

Is this the best way to solve the issue?

No: waiting for all uploads before cleanup is the right fence, but the PR must record the first temporal rejection rather than select the first rejected array element.

Full review comments:

• [P2] Rethrow the earliest upload failure — src/worker/session-log-archive.ts:67-69
allSettled returns results in input order, not settlement order. If summary.json rejects first and events.ndjson rejects later, this selects and throws the events error, contrary to preserving the original failure. This was already raised for this unchanged head; capture the first rejection as it occurs and add a staged dual-rejection test.
  Confidence: 0.99
  Late finding: first raised on code an earlier review cycle already covered.

Overall correctness: patch is incorrect
Overall confidence: 0.99

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

• remove merge-risk: 🚨 other: Current PR review selected no merge-risk labels.

Label justifications:

• P2: A failure-path diagnostic error can mislead operators during a bounded R2 archive failure, while the cleanup behavior itself remains narrowly scoped.
• 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 the Worker archive service, but the supplied terminal trace and added test exercise it with a Map-backed put/delete substitute rather than a configured Worker SESSION_LOGS binding and real R2 client; the after-fix result is therefore supplemental 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

What I checked:

• Introduced error selection: The introduced code waits for all uploads, then uses results.find; Promise.allSettled preserves input order, so a later event-upload rejection is selected over an earlier summary-upload rejection. (src/worker/session-log-archive.ts:67, 76a93be04460)
• Test coverage gap: The added test has only one rejected upload, so it proves cleanup but cannot detect the dual-rejection ordering error. (tests/session-log-archive.test.ts:179, 76a93be04460)
• Pre-PR behavior: The merged session-ledger implementation still used Promise.all with no failure cleanup; the fetched main/base commit therefore does not already contain this fix. (src/worker/session-log-archive.ts:53, b853216dcdf0)
• Feature history: History identifies the structured session-event storage work and later session-ledger hardening as the relevant merged lineage. (src/worker/session-log-archive.ts:17, 72a737f68be9)
• Supplied behavior evidence: The PR body shows a post-patch terminal trace against a Map-backed R2 substitute; it is useful regression evidence but not a configured Worker SESSION_LOGS binding or real R2-client run. (76a93be04460)

Likely related people:

• Vincent Koc: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
• Peter Steinberger: 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 the earliest rejection while retaining all-settled cleanup, then add a staged dual-rejection regression.
• Attach redacted after-fix Worker/R2 output showing cleanup, no archive row, and the preserved earliest error; update the PR body for re-review.

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 (15 earlier review cycles; latest 8 shown)

• reviewed 2026-08-31T03:14:39.019Z sha 76a93be :: needs real behavior proof before merge. :: [P2] Preserve the first-upload failure
• reviewed 2026-08-31T05:32:56.495Z sha 76a93be :: needs real behavior proof before merge. :: [P2] Preserve the earliest rejected R2 upload
• reviewed 2026-08-31T07:58:42.059Z sha 76a93be :: needs real behavior proof before merge. :: [P2] Rethrow the first settled upload failure
• reviewed 2026-08-31T11:52:37.733Z sha 76a93be :: needs real behavior proof before merge. :: none
• reviewed 2026-09-01T04:22:53.574Z sha 76a93be :: needs real behavior proof before merge. :: [P2] Preserve the first settled upload failure
• reviewed 2026-09-01T08:11:27.597Z sha 76a93be :: needs real behavior proof before merge. :: [P2] Preserve the earliest upload failure
• reviewed 2026-09-01T08:40:28.274Z sha 76a93be :: needs real behavior proof before merge. :: [P2] Rethrow the first settled upload error
• reviewed 2026-09-01T10:58:21.794Z sha 76a93be :: needs real behavior proof before merge. :: [P2] Preserve the earliest upload failure