[Improve] Keep compact communication footers current with coding activity - #2415
[Improve] Keep compact communication footers current with coding activity#2415roomote-roomote[bot] wants to merge 13 commits into
Conversation
|
No code issues found. See task
Reviewed c0c9f88 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![roomote-community[bot]](https://avatars.githubusercontent.com/u/211522643?s=60&v=4){kind=small}
|
Latest simplification: d689adba. Removed generic footer deletion of All 316 communication tests, communication TypeScript, and pre-push checks pass locally. Fresh GitHub Test, Type Check, Lint and Knip pass; Roomote review, JavaScript CodeQL analysis and build are still running. Previous mention-footer removal and Redis mock fixes remain intact. |
…-footer-2zog0frjgail2
- Compare Slack footer text after decoding the entities Slack escapes on read-back, so unchanged footers no longer trigger an edit every tick. - Report what each refresh learned: active destinations re-check every 30 seconds, idle ones every 5 minutes, and settled or unresolvable ones unregister instead of polling for the record's whole lifetime. - Lease claimed targets past the scheduler cadence, run one pass at a time under a job lock with a deadline, and reschedule explicitly. - Resolve state outside the destination lock and hold it only for the provider edit; refreshes try the lock once, deliveries wait longer, and the Slack details toggle tells the user when the thread is busy. - Check the KEEPTTL XX result so an edit on an expired record strips its own footer instead of leaving an orphan. - Strip the footer from the previous source-control comment when a new turn relocates it. - Keep the thread's own task selected when a task footer opens the owning Session, and keep caller-owned navigation such as setup links. - Carry pull request links a refresh cannot re-derive until the database says they are closed; text providers post the caller's footer as-is. - Resolve every linked task's latest run in one query. - Document the compact footer on the provider pages.
| return 'gone'; | ||
| } | ||
| await assertLock(); | ||
| await setSourceControlFooterRecord({ ...latest, footerText }, true); |
There was a problem hiding this comment.
The refresh path still has a check-then-write lease gap. After assertLock() at line 1278 succeeds, this unconditional pointer update can run after the lease expires and another delivery has installed a newer carrier. It then repoints refresh to this historical comment, so a later tick restores its footer. Make setSourceControlFooterRecord owner-fenced in the same Redis operation (and strip this edited footer if that conditional write loses), rather than relying on the preceding GET.
There was a problem hiding this comment.
Fixed in 5471551. setSourceControlFooterRecord now takes the lease and checks ownership in the same Redis operation (Lua, like the managed-provider record write), for both the initial delivery write and the post-edit keepTtl write. A refresh whose fenced write loses re-reads the pointer and strips the footer it just applied when a newer carrier is current. Covered by a test where the lease lapses mid-edit and a newer reply takes over.
Write the source-control footer record and verify lock ownership in one Redis operation, for both initial delivery and the post-edit refresh write, so a lease that lapses after assertLock cannot repoint refresh at a historical comment. A refresh whose write loses strips the footer it just applied when a newer carrier exists.
| target.channelId, | ||
| target.threadId, | ||
| ).catch(() => undefined); | ||
| if (current !== undefined && current?.messageId !== latest.messageId) |
There was a problem hiding this comment.
A lease-loss competitor can update the same carrier rather than relocate it: a resumed review-thread turn or replaceReply keeps messageId but writes a newer body/footer. If this refresh's provider edit completes after that turn, it overwrites the newer content; this ID-only check then skips cleanup because both records are latest.messageId. Treat a different stored body/footer on the same ID as a newer carrier too, and restore that record's footer/body. rememberThreadComment has the same ID-only cleanup predicate.
There was a problem hiding this comment.
Fixed in 7f63308. Both cleanup paths now go through one helper: after a fenced write loses, it re-reads the record and, when the current carrier is this same comment id with a different recorded body or footer, rewrites the comment with that record's body and footer instead of skipping. A different id (or no record) still strips only this comment's own footer. Tests cover a refresh and a reply each losing the lease to a replaceReply on the same comment.
A competitor that loses the lease race can rewrite the same comment rather than relocate the footer. Treat a different recorded body or footer on the same comment id as the newer carrier and put its content back, in both the delivery and refresh cleanup paths.
| const current = await getSourceControlFooterRecord( | ||
| params.channelId, | ||
| params.threadId, | ||
| ).catch(() => undefined); | ||
| if (current === undefined) return; | ||
| if (!current || current.messageId !== params.mine.messageId) { |
There was a problem hiding this comment.
restoreCompetingCarrier reads the successor record and then updates the provider without holding a new lock. A second replaceReply can acquire the lock and persist a newer body/footer after this read but before params.update() completes; this stale recovery then overwrites that newer comment while its pointer still advertises the newer content. The next refresh can skip the edit because the recorded footer is already current, leaving the old body visible. Reacquire/yield on the destination lock and re-read the record immediately before the restoration edit so the recovery cannot race a subsequent owner.
There was a problem hiding this comment.
Fixed in f704aab. The restoration now runs under the destination lock: it reuses the caller's lease when that lease is still held, otherwise it acquires a fresh lock, and it re-reads the record and asserts the lease immediately before the restoring edit. A test starts a third replaceReply while the restoration edit is in flight and checks it waits for the lock, so its content is what ends up on the comment.
There was a problem hiding this comment.
Holding the lock improves the prior race, but the restoration is still not fenced across the provider request. After assertLock() at line 924 succeeds, a slow params.update() can outlive the 30-second lease (or its renewal can fail); another owner can then acquire the lock and persist a newer same-comment body/footer before this stale restoration completes. This update then overwrites that newer body while Redis still points at it, and a later refresh can skip because the recorded footer is current. Treat a post-edit lease loss as a failed restoration or re-check/recover after the provider update instead of assuming the pre-edit lock assertion remains valid.
There was a problem hiding this comment.
Fixed in feffb41. After the restoring edit, the recovery asserts the lease again. A lease still held after the edit proves nothing newer landed; a lost lease treats the restoration as unproven and starts over under a fresh lock (up to three passes), each pass comparing the current record against what the recovery last wrote and only editing when they differ. Test: the restoring edit itself outlives the lease while a third owner rewrites the comment, and the follow-up pass puts that owner's content back.
There was a problem hiding this comment.
The retry cap still leaves the same race reachable. If each restoring provider edit outlives its lease and a newer same-comment replaceReply wins it, the third inner attempt returns retry and this loop exits without a final restore or cleanup. That final stale provider response can overwrite the newer body while Redis still records the newer body/footer; subsequent refreshes see the footer text as current and skip the edit, so the stale body remains. Retry exhaustion needs a safe terminal cleanup/reconciliation rather than only logging.
There was a problem hiding this comment.
Fixed in c0c9f88. The retry loop is gone. A restoration whose lease is lost after its edit no longer guesses again: under a fresh fenced write it blanks the record's footerText (body stays the newest owner's) and schedules a refresh. The next refresh pass sees the footer as stale, rewrites the comment from the record under its own lease, and only records the footer through its fenced post-edit write, so a stale provider response can never end as the final state while Redis says the footer is current. Test: the restoring edit outlives its lease while a third owner rewrites the comment; the record ends with that owner's body and an unknown footer, a refresh is scheduled, and the next pass writes the correct body and footer.
Recovery after a lost footer lease re-reads the record and edits the comment while holding the destination lock (the caller's lease when it still holds it, otherwise a fresh one), so a later owner cannot persist newer content between the read and the edit and be painted over.
A restoring provider edit can outlive its lease. Only a lease still held after the edit proves nothing newer landed meanwhile; otherwise the restoration starts over under a fresh lock, up to three times, comparing the record against what it last wrote.
When a restoring edit's lease is lost after the edit, stop guessing: mark the record's footer unknown under a fenced write and schedule a refresh, so the next pass rewrites the comment from the record with its own fenced post-edit write. The record, never a stale provider response, decides what the comment shows.
2 participants
What changed
Why this change was made
Users need to tell whether delegated coding is still running and navigate to it without a crowded footer or waiting for another assistant reply. Shared formatting and refresh paths keep communication providers consistent.
Impact
Refresh is eventually consistent, not instantaneous: the scheduler runs every 30 seconds, with bounded batches and retries that can add delay under load.
Navigation limitation: the existing Session task-list panel has no supported deep-link state. Zero and multiple running tasks therefore link to global
/tasks, not a Session-filtered list. Single-task links remain Session-scoped. No route was invented.Latest URL simplification on d689adb: all 316 communication tests, package TypeScript, and pre-push checks pass locally. Fresh GitHub Test, Type Check, Lint and Knip pass; remaining analysis/review/build checks are running. Prior 70dca67 passed 556 affected local tests and all GitHub checks including Roomote review. No browser screenshots or live external-provider end-to-end validation were performed.