feat(android): durable offline command outbox for chat sends#100290
Conversation
openclaw-barnacle
Bot
added
app: android
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12edbf9c20
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch (err: Throwable) { | ||
| OutboxSendResult.TransportFailure(err.message ?: "send failed") |
There was a problem hiding this comment.
When a queued send reaches the gateway but is rejected at the RPC layer, GatewaySession.request throws for ok:false responses (apps/android/app/src/main/java/ai/openclaw/app/gateway/GatewaySession.kt:351-359), and chat.send uses that path for cases like archived/deleted sessions (src/gateway/server-methods/chat.ts:3900-3903). This catch treats those application rejections as TransportFailure, so the row is put back in Queued with retryCount unchanged and health forced false, leaving a permanently invalid command to retry forever instead of surfacing as failed/deletable after the configured attempts.
Useful? React with 👍 / 👎.
| // Same monotonic clamp as enqueue: a retried row re-joins the end of the FIFO queue. | ||
| val createdAt = maxOf(nowMs, (dao.maxCreatedAt(gateway) ?: Long.MIN_VALUE) + 1) | ||
| dao.requeueForRetry(id = id, createdAtMs = createdAt, status = ChatOutboxStatus.Queued.dbValue) |
There was a problem hiding this comment.
For a row that reaches Failed after a terminal ACK, user Retry only refreshes status/timestamp and keeps the same row id. The gateway caches chat.send results by chat:<runId> before accepting new work (src/gateway/server-methods/chat.ts:3841-3846), while the send path uses this row id as the idempotency key, so retrying a terminal timeout/error replays the cached failure instead of starting a fresh turn. Keep the stable key for ambiguous redelivery, but rotate it for explicit user retry after terminal failure.
Useful? React with 👍 / 👎.
| commandOutbox?.let { | ||
| runCatching { it.deleteForSession(key) } | ||
| publishOutbox() |
There was a problem hiding this comment.
Rows queued before the gateway hello are stored with sessionKey == "main", and the flush path later maps them to the canonical appliedMainSessionKey. If the gateway deletes that canonical main session, this exact-key purge misses the pre-hello "main" rows, so they remain visible and can be flushed into the deleted session on the next reconnect. When key is the current canonical main key, purge the "main" alias as well.
Useful? React with 👍 / 👎.
|
Codex review: needs real behavior proof before merge. Reviewed July 5, 2026, 4:41 PM ET / 20:41 UTC. Summary PR surface: Other +1581. Total +1581 across 15 files. Reproducibility: yes. for review findings: the destructive migration, retry-key reuse, RPC-rejection catch, main-alias purge, live-ordering, composition-scope handoff, and timeline anchoring issues are source-reproducible from the PR head and current gateway/controller contracts. Review metrics: 1 noteworthy metric.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review findings
Review detailsBest possible solution: Keep the Android text-outbox direction, but require non-destructive durable storage or explicit maintainer acceptance of the data-loss contract, fix the delivery/scroll blockers, and add redacted Android emulator or device proof before merge. Do we have a high-confidence way to reproduce the issue? Yes for review findings: the destructive migration, retry-key reuse, RPC-rejection catch, main-alias purge, live-ordering, composition-scope handoff, and timeline anchoring issues are source-reproducible from the PR head and current gateway/controller contracts. Is this the best way to solve the issue? No. A Room-backed Android text outbox is a plausible staged design, but this head is not the best mergeable form until delivery semantics, timeline behavior, storage/security decisions, and real Android proof are resolved. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 5e0504aa437e. Label changesLabel justifications:
Evidence reviewedPR surface: Other +1581. Total +1581 across 15 files. View PR surface stats
Security concerns:
Acceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
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
Review history (8 earlier review cycles)
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
e0701c7 to
a19a3d4
Compare
df9eb0e to
b658f9b
Compare
12edbf9 to
675432d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 675432df45
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @Database( | ||
| entities = [CachedSessionEntity::class, CachedMessageEntity::class], | ||
| version = 1, | ||
| entities = [CachedSessionEntity::class, CachedMessageEntity::class, OutboxCommandEntity::class], |
There was a problem hiding this comment.
Adding OutboxCommandEntity to this database makes the existing destructive migration apply to durable user input: when a user has queued offline commands and installs a later build that bumps this Room schema, .fallbackToDestructiveMigration(dropAllTables = true) will delete those rows before they can be sent, expired, or manually removed. This store is no longer only disposable transcript cache, so the outbox needs a migrated/non-destructive owner before it ships.
Useful? React with 👍 / 👎.
| sendScope.launch { | ||
| val accepted = onSend(message) |
There was a problem hiding this comment.
When the sheet/screen leaves composition right after tapping Send (for example dismissal, navigation, or rotation), rememberCoroutineScope is cancelled; this new coroutine now owns onSend(message)/sendChatAwaitAcceptance after the draft has already been cleared, so the message can be cancelled before the runtime queues or sends it and the restore path may never run. The previous path handed off to runtime scope immediately; keep dispatch in ViewModel/runtime scope and only report the acceptance result back for draft restoration (the same new pattern in ChatScreen needs the same treatment).
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
b658f9b to
f2d1528
Compare
675432d to
75a7446
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75a74464d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var attempts = item.retryCount | ||
| while (true) { | ||
| val error = | ||
| when (val result = attemptOutboxSend(item)) { |
There was a problem hiding this comment.
Separate from the already-noted user Retry path, the automatic retry loop also calls attemptOutboxSend(item) with the same row id each time, and that id is sent as the chat.send idempotency key. The gateway caches chat:<id> terminal results before later duplicate checks (src/gateway/server-methods/chat.ts:3841-3846, src/gateway/server-methods/chat.ts:5771-5784), so a transient terminal error/timeout ACK is replayed for attempts 2 and 3 instead of re-running the turn, burning the configured attempts and parking the row as Failed immediately. Rotate an attempt key for terminal retries, or treat terminal ACKs as non-retryable.
Useful? React with 👍 / 👎.
f2d1528 to
b9fc80e
Compare
75a7446 to
9e4a937
Compare
b9fc80e to
17b5cd4
Compare
9e4a937 to
bc36e19
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc36e19dfe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var attempts = item.retryCount | ||
| while (true) { | ||
| val error = | ||
| when (val result = attemptOutboxSend(item)) { |
There was a problem hiding this comment.
If the user disconnects or starts pairing a different gateway after the flush loop loads a row but before this call, onGatewayScopeChanging() has already flipped health/scope, yet this path still sends the captured old-gateway row once before the later scope checks run. That can replay a queued command into a connection that is being torn down or has just been replaced; add the same _healthOk/currentCacheScope() == flushScope guard immediately before the first attemptOutboxSend.
Useful? React with 👍 / 👎.
17b5cd4 to
a259a40
Compare
bc36e19 to
1366861
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 136686133a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| val items = | ||
| buildList { | ||
| // reverseLayout: index 0 renders bottom-most; queued commands are the newest user input. | ||
| outboxItems.asReversed().forEach { item -> add(ChatTimelineItem.OutboxCommand(item)) } |
There was a problem hiding this comment.
When a text send is accepted offline, this inserts the queued bubble at index 0, but the later latestUserMessage calculation still only considers persisted Message rows, and ChatReaderState.onTimelineChanged only auto-follows a new user turn when latestUserMessageVersion changes. In a chat that already has a user message and is following the read anchor, adding a queued row changes latestContentVersion but not the latest user version, so the reader scrolls back to the previous prompt instead of the new queued bubble; the user can miss that the offline command was accepted.
Useful? React with 👍 / 👎.
1366861 to
91b5abc
Compare
91b5abc to
fe50540
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe50540022
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| runCatching { outbox.delete(item.id) } | ||
| publishOutbox() | ||
| return OutboxSendOutcome.Sent |
There was a problem hiding this comment.
When an offline row flushes and chat.send returns a non-terminal {runId, status:"in_flight"} ack, this branch deletes the row and reports Sent without registering the returned run in pendingRuns. For a queued message in the currently visible session, subsequent chat deltas are ignored because handleChatEvent only renders run IDs already in pendingRuns, and the composer stays enabled with pendingRunCount == 0 while the gateway is still working. Mirror the live-send path by adopting/arming the accepted run before removing the queued bubble.
Useful? React with 👍 / 👎.
|
Merged via squash.
|
1 participant
Stacked on the Android chat offline cache branch (#100227,
feat/android-chat-offline-cache); lands after it.Part of #46664
What Problem This Solves
Text commands typed while the gateway is unreachable are currently refused with an error and lost. Users routinely open chat while disconnected (transit, flaky Wi-Fi, gateway restart) and expect what they typed to be delivered once the connection returns.
Why This Change Was Made
A durable outbox lets users keep issuing text commands offline and have them delivered in order after reconnect, without inventing a second reachability source or a new store: it extends the existing chat cache database (schema v2, destructive migration per that DB's established contract) and the existing gateway-health transition as the flush trigger. Every row's UUID is its
chat.sendidempotency key, so delivery is at-least-once with gateway-side dedup — history replaces each queued bubble exactly once. Conservative defaults: 50 queued per gateway, 48h expiry to "failed (expired)" instead of firing stale commands, 3 send attempts with backoff, transport drops keep rows queued without burning attempts,sendingrows revert toqueuedon startup, and pairing/auth resets purge the outbox with the cache. Text-only v1; attachments stay online-only.User Impact
Evidence
RoomChatCommandOutboxTest(Robolectric, 10 tests): persistence, FIFO under same-millisecond clock collisions, MAX_QUEUED refusal, 48h boundary expiry, sending→queued recovery, retry-refresh, gateway scoping, purges.ChatControllerOutboxTest(12 tests): enqueue-while-offline across controller recreation, ordered flush with idempotency keys asserted, ack removes row with exactly one history bubble, rejected→failed-after-3, transport-failure requeue, main-alias rows flushed to the canonical main session, purge on session delete.cd apps/android && ./gradlew :app:testPlayDebugUnitTest— full lane green; ktlint clean on touched files; native i18n inventory synced.Follow-up: voice capture (
MicCaptureManager) still buffers recognized turns in an in-memory FIFO while offline; its queue is coupled to per-turn reply correlation and TTS playback, so routing it into the durable outbox is deferred to a dedicated change. No on-device screenshot pass yet (unit/Robolectric proof).Update (rebase onto the reworked cache base): the base branch's cache API was rebuilt upstream around explicit gateway-scope binding (
ChatCacheScope); the outbox is now re-expressed on that model — enqueue/publish/flush bind to the captured scope, a pairing/connection switch mid-flush stops the flush and clears presented rows, per-item retries re-check health and scope around each backoff (old-gateway text can never replay into a new gateway), andupdateStatusis a claiming write so a row deleted through any path is never resent. Squashed to a single commit on the new base; 24 outbox tests green plus the full Play unit lane. One autoreview finding consciously rejected with inline rationale: destructive migration for the outbox table is the established design contract for this database (losing at most a few unsent commands at a rare schema-bump boundary is the accepted trade-off, documented at the site).