Skip to content

fix: keep tail alive and replay timed-out messages - #126

Merged
steipete merged 23 commits into
openclaw:mainfrom
hannesrudolph:codex/fix-tail-handler-timeouts
Jul 17, 2026
Merged

fix: keep tail alive and replay timed-out messages#126
steipete merged 23 commits into
openclaw:mainfrom
hannesrudolph:codex/fix-tail-handler-timeouts

Conversation

@hannesrudolph

@hannesrudolph hannesrudolph commented Jul 13, 2026

Copy link
Copy Markdown
Member

Problem

A message create, update, or delete handler can exceed its deadline while holding Discrawl's writable SQLite connection. The outer failure recorder can then miss the same connection deadline, leaving no durable exact-message recovery identity. Delete failures also need different replay semantics from fetch-and-upsert.

Current scope

  • Keep Client.Tail as the sole durable owner for message-scoped returned errors, panics, and timeouts.
  • Persist normalized create, update, or delete identities in the failure ledger.
  • If bounded ledger persistence cannot acquire the database, atomically spool a private, content-free fallback and import it once after restart.
  • Replay create/update with exact REST fetch semantics and replay delete by marking canonical state deleted without synthetic events, cursor movement, publishing, or tail:last_event movement.
  • Preserve repeated unresolved occurrences with receipts, bounded joins, and privacy-safe diagnostics.
  • Add the bounded discrawl tail --replay-failures-only [--replay-limit N] maintenance path. The earlier exact-selector CLI surface was removed as unnecessary dead weight.
  • Protect fallback directories and files on Unix and Windows; use no-replace moves and handle-relative Windows ACL validation.

The original fourteen commits remain authored by @hannesrudolph. Maintainer repair commits tighten recovery, docs, tests, and Windows security without changing contributor authorship.

Exact-head evidence

Current head: 32c7aa657aaa58b2ede34549e6b0029ce429a32d

Full local gate:

  • go test -count=1 ./... -coverprofile=/private/tmp/discrawl-pr126-cover.out — PASS; filtered coverage 85.2% (floor 85.0%).
  • go test -count=1 -race ./... — PASS.
  • golangci-lint run ./..., go vet ./..., staticcheck ./..., deadcode -test ./..., gofumpt -d ., and gosec with the repository CI exclusions — PASS / no findings.
  • go mod verify, go mod tidy -diff, and final git diff --check — PASS / clean.

Protocol-real timeout fixtures:

  • GOWORK=off go test ./internal/discord -run 'TestTailContinuesAfterHandlerFailure/timeout|TestTailMessageUpdateFailureUsesRefetchedMetadata/timeout|TestTailMessageDeleteFailureRecordsExactlyOnce/cooperative_timeout' -count=1 -v — create, update, and delete timeout paths all PASS against real local WebSocket Gateway and REST servers.

Restart and replay proof used two separate processes and the real built CLI:

  • Process one durably spooled three canonical fallbacks, then exited before import: spooled=3 process_exit_before_import=true.
  • Before restart: three private spool artifacts; no ledger rows; one message still present in FTS, embeddings, and jobs; saved cursor/event markers intact.
  • discrawl tail --replay-failures-only --replay-limit 25 imported exactly three artifacts, recovered the delete, and safely deferred unauthenticated create/update fetches.
  • After replay: zero spool artifacts; three receipts; the message was marked deleted and its FTS/embedding/job state was removed; no message event was synthesized; cursor and tail-event markers were unchanged.
  • A second process found only the two unresolved create/update entries, created no duplicate receipts, and left the resolved delete unchanged.

Native Windows proof ran on Crabbox AWS Windows lease cbx_a41aecb6e3af (violet-crayfish), run run_4f1ff207f6f0, at the exact final source tree. The Windows test executable SHA-256 was 5f8d6ebc2bac3cdebe10594d7fca2372c268b82a3e3f9a3fa094ec68c74455d8; canonical/private persistence, DACL protection, no-replace rename, and long-path tests all passed. The CLI executable SHA-256 was d58efd1faeaf4d69b3b5522269653d308c3a12450d12ca2dd244350a0ff1ee88; native --version reported 0.11.6-0.20260717072229-32c7aa657aaa, and native tail --help exposed the bounded replay flags.

AutoReview was run repeatedly over the repair and split review slices because the complete branch exceeds its single-bundle limit. Accepted findings were fixed; final review is clean.

Authenticated Discord boundary

The local environment has no enabled Discord bot token or designated disposable guild/channel. Therefore a successful create/update replay against Discord's hosted REST API, plus hosted Gateway delivery across a real process restart, still requires an authenticated Discord run. Local proof does not claim that hosted clause. It does exercise the actual HTTP/WebSocket protocol paths and the production SQLite/fallback/replay implementation rather than mocked store interfaces.

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P1 Urgent regression or broken agent/channel workflow affecting real users now. merge-risk: 🚨 message-delivery 🚨 Merging this PR could drop, duplicate, misroute, suppress, or wrongly target messages. labels Jul 13, 2026
@clawsweeper

clawsweeper Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge. Reviewed July 17, 2026, 3:52 AM ET / 07:52 UTC.

Summary
The PR makes Discord tail failures durable and replayable, adds content-free fallback spooling, coordinates repair shutdown, and exposes a bounded replay-only CLI mode.

Reproducibility: no. failing current-main run is included, but the pre-fix ownership and deadline interaction is source-reproducible and the branch supplies focused protocol-real timeout fixtures for all three affected message event types.

Review metrics: 3 noteworthy metrics.

  • Patch surface: 31 files; 10,976 additions, 325 deletions. The change spans concurrency, storage, replay, CLI, documentation, and platform-specific security, so focused area-owner review remains important.
  • Commit history: 23 commits. The branch contains fourteen contributor commits followed by nine focused repair and validation commits, showing substantial iterative review work.
  • Runtime proof: 3 message event kinds; 2-process restart proof; native Windows run. The evidence covers create, update, and delete timeout handling plus restart idempotency and Windows filesystem protections.

Merge readiness
Overall: 🦐 gold shrimp
Proof: 🦞 diamond lobster
Patch quality: 🦐 gold shrimp
Result: ready for maintainer review.

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

Rank-up moves:

  • Remove the PR-added CHANGELOG.md entry.
  • [P2] Let the exact-head required checks complete successfully.
  • If a disposable guild is available, capture an authenticated hosted Discord create/update restart-and-replay transcript with private identifiers redacted.

Risk before merge

  • [P1] The final fail-closed recovery policy can intentionally stop the tail when neither the ledger nor the protected fallback can preserve an exact identity; this protects delivery integrity but changes the availability failure mode.
  • [P1] The proof does not include authenticated create/update replay against Discord's hosted REST API and Gateway across a real process restart, so that last external integration boundary remains unverified.
  • [P1] The patch changes 31 files and more than ten thousand added lines across tail concurrency, persistent recovery, replay semantics, CLI locking, and platform-specific filesystem security, leaving meaningful message-delivery regression risk despite extensive focused coverage.

Maintainer options:

  1. Remove the changelog entry (recommended)
    Delete the PR-added unreleased CHANGELOG.md line, retain the recovery implementation unchanged, and revalidate the exact head before merge.
  2. Require hosted Discord proof
    Pause merge until an authenticated disposable Discord setup demonstrates create and update replay across a real process restart.
  3. Accept the external-boundary gap
    Merge after the mechanical changelog fix based on the protocol-real local fixtures, two-process CLI proof, and native Windows evidence, while explicitly owning the remaining hosted integration risk.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Remove the PR-added unreleased entry from CHANGELOG.md without changing recovery code or tests, then run git diff --check.

Next step before merge

  • [P2] A narrow mechanical repair remains: remove the release-owned changelog entry without altering the extensively validated recovery implementation.

Security
Cleared: The diff adds no workflow, dependency-source, lockfile, publishing, or secret-access changes, and its content-free fallback records are protected with ownership and permission or DACL validation.

Review findings

  • [P2] Remove the release-owned changelog entry — CHANGELOG.md:13
Review details

Best possible solution:

Preserve the fail-closed durable-recovery design, remove the release-owned changelog edit, land only after exact-head checks complete, and perform an authenticated hosted Discord restart/replay smoke test when a disposable guild is available.

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

No failing current-main run is included, but the pre-fix ownership and deadline interaction is source-reproducible and the branch supplies focused protocol-real timeout fixtures for all three affected message event types.

Is this the best way to solve the issue?

Yes for the technical recovery approach: a single durable owner, content-free protected fallback, and event-aware exact replay form a narrow integrity-preserving design. The release-owned changelog edit should be removed, and hosted authenticated proof would further reduce integration risk.

Full review comments:

  • [P2] Remove the release-owned changelog entry — CHANGELOG.md:13
    Delete this unreleased entry before merge. This repository keeps CHANGELOG.md release-owned, so normal implementation PRs should provide release-note context in the PR body rather than editing the file; this was also the unresolved finding from the preceding review cycle.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.97

AGENTS.md: not found in the target repository.

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

Label changes

Label changes:

  • add merge-risk: 🚨 availability: The intentional fail-closed path can stop the tail when durable recovery cannot be secured, changing process availability under storage failure.
  • remove proof: sufficient: Current real behavior proof status is not_applicable, not sufficient.

Label justifications:

  • P1: The PR addresses potential loss of durable recovery identity for live Discord create, update, and delete failures, directly affecting a core channel workflow.
  • merge-risk: 🚨 message-delivery: Incorrect timeout ownership or replay semantics could lose, duplicate, or incorrectly recover Discord message mutations.
  • merge-risk: 🚨 availability: The intentional fail-closed path can stop the tail when durable recovery cannot be secured, changing process availability under storage failure.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The external-contributor proof gate does not apply to this MEMBER-authored PR; nevertheless, the body provides strong exact-head protocol fixtures, two-process CLI restart output, and native Windows Crabbox evidence, while clearly disclosing the missing authenticated hosted Discord run.
Evidence reviewed

Acceptance criteria:

  • [P1] git diff --check.
  • [P1] git diff -- CHANGELOG.md.

What I checked:

  • Current main does not contain the proposed recovery implementation: The PR adds the fallback spool, replay engine, platform-specific security and rename implementations, and associated tests relative to current main 91aa80d. (internal/store/tail_failure_fallback.go:1, 32c7aa657aaa)
  • Exact-head timeout coverage: The PR body records passing protocol-real WebSocket and REST timeout fixtures for message create, update, and delete paths at the exact final head. (internal/syncer/syncer_tail_test.go:1, 32c7aa657aaa)
  • Restart and replay proof: Two separate processes exercised the real CLI and production SQLite implementation: three fallback artifacts were imported once, delete state was recovered without synthetic events or cursor movement, and a second replay created no duplicate receipts. (internal/syncer/tail_replay.go:31, 32c7aa657aaa)
  • Native Windows security proof: The exact final tree passed native Windows tests for private persistence, DACL validation, no-replace rename behavior, and long paths on Crabbox run run_4f1ff207f6f0. (internal/store/tail_failure_security_windows.go:1, 32c7aa657aaa)
  • Prior review blocker remains: The exact-head file list still includes a PR-authored unreleased entry in CHANGELOG.md, despite the preceding review requesting its removal under the repository's release-owned changelog policy. (CHANGELOG.md:13, 32c7aa657aaa)
  • Maintainer-sponsored technical direction: The current branch includes nine focused repair, documentation, test, Windows-security, and fail-closed commits by steipete after the contributor's original implementation, providing strong evidence that the recovery direction has active area-owner sponsorship. (internal/syncer/tail.go:15, 32c7aa657aaa)

Likely related people:

  • steipete: Authored the latest narrowing, durability, Windows-security, queue-overflow, ownership-validation, and fail-closed commits on the affected recovery paths, with prior history across Discord tail and CLI code. (role: recent area contributor and repairer; confidence: high; commits: cfed4abdf81f, 9c4dd48164f8, 8976614bdd63; files: internal/discord/client.go, internal/syncer/tail.go, internal/syncer/tail_replay.go)
  • vincentkoc: Earlier feature-history review ties this contributor to the initial Discord tail, gateway, and CLI implementation that this PR substantially extends. (role: original tail-area contributor; confidence: medium; commits: 5c60af1, 1198875, 772c06f; files: internal/discord/client.go, internal/syncer/tail.go, internal/cli/admin_commands.go)
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.
Review history (13 earlier review cycles; latest 8 shown)
  • reviewed 2026-07-13T23:27:12.490Z sha c3e2369 :: needs changes before merge. :: [P1] Record panicked message events for exact replay
  • reviewed 2026-07-13T23:52:04.520Z sha d8062ea :: needs maintainer review before merge. :: none
  • reviewed 2026-07-14T00:14:32.167Z sha dae939e :: needs maintainer review before merge. :: none
  • reviewed 2026-07-14T00:33:44.902Z sha dae939e :: needs maintainer review before merge. :: none
  • reviewed 2026-07-14T02:51:34.237Z sha 511dbe1 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-14T03:02:58.854Z sha 511dbe1 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-15T11:04:08.392Z sha a9f843a :: needs maintainer review before merge. :: none
  • reviewed 2026-07-17T06:26:39.396Z sha 437055c :: found issues before merge. :: [P2] Remove the release-owned changelog entry

@hannesrudolph hannesrudolph changed the title fix: keep tail alive after handler failures fix: keep tail alive and replay timed-out messages Jul 13, 2026
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. merge-risk: 🚨 message-delivery 🚨 Merging this PR could drop, duplicate, misroute, suppress, or wrongly target messages. and removed status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. merge-risk: 🚨 message-delivery 🚨 Merging this PR could drop, duplicate, misroute, suppress, or wrongly target messages. labels Jul 13, 2026
@hannesrudolph

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 13, 2026

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 rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jul 13, 2026
@hannesrudolph

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 13, 2026

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 rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jul 13, 2026
@hannesrudolph

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 14, 2026

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 rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. merge-risk: 🚨 message-delivery 🚨 Merging this PR could drop, duplicate, misroute, suppress, or wrongly target messages. labels Jul 14, 2026
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jul 17, 2026
@steipete
steipete force-pushed the codex/fix-tail-handler-timeouts branch from 437055c to 32c7aa6 Compare July 17, 2026 07:47
@clawsweeper clawsweeper Bot added merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. and removed proof: sufficient Contributor real behavior proof is sufficient. labels Jul 17, 2026
@steipete
steipete merged commit d865b56 into openclaw:main Jul 17, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 message-delivery 🚨 Merging this PR could drop, duplicate, misroute, suppress, or wrongly target messages. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants