fix(slack): prevent activity indicator race from overwriting final message on terminal phase

[kristiancailer]

What type of PR is this?

/kind bug

What this PR does / why we need it:

Fixes a race condition between the activity indicator loop and the terminal-phase path that was introduced when we started editing the progress message in-place with the final result. Both paths now target the same Slack message, so a concurrent UpdateActivityIndicator call could overwrite the final succeeded/failed content with stale activity indicator text.

The race window: the activity goroutine reads activityState.MessageTS and drops the mutex → the terminal path writes the final result via UpdateMessage and then calls clearActivityState → the activity goroutine proceeds to call UpdateMessage with intermediate content, overwriting the final result.

Fix (two parts):

1. In the terminal path, call clearActivityState before UpdateMessage so that any concurrent activity loop sees the state as gone before the final content is written.
2. In UpdateActivityIndicator, add a re-check after building the message (and before the API call) that the activity state still exists and still targets the same MessageTS. If the terminal path cleared it in the meantime, bail out.

Which issue(s) this PR is related to:

N/A

Special notes for your reviewer:

• The existing LastText persistence at line 568 already uses this "re-check MessageTS after the API call" pattern — the new guard applies the same pattern before the API call.
• In the terminal path, clearActivityState is now called unconditionally before UpdateMessage. If UpdateMessage fails and we fall through to the PostThreadReply fallback, the activity state is already cleared — this is fine because the task is terminal and the activity loop should not be updating anything.
• New test TestSlackTaskReporter_ActivitySkipsUpdateWhenStateCleared verifies that UpdateActivityIndicator makes zero Slack API calls when the activity state has been cleared between reading it and calling the API.

Does this PR introduce a user-facing change?

NONE

[greptile-apps]

Greptile Summary

This PR fixes a race condition in the Slack reporter where the activity-indicator goroutine could overwrite the final succeeded/failed message by calling UpdateMessage with stale intermediate content after the terminal path had already written the result. The fix has two parts: (1) clearActivityState is now called before UpdateMessage in the terminal path so any concurrent activity loop sees the state as gone first, and (2) UpdateActivityIndicator adds a re-check under the mutex after building the message but before the API call to bail out if the state was cleared in the meantime.

Confidence Score: 5/5

Safe to merge — the fix correctly closes the primary race window; the only remaining concern is a narrow TOCTOU gap that is an inherent limitation of the unlocked-API-call pattern.

All findings are P2. The core logic is sound and matches established patterns in the codebase. The new test covers the deterministic scenario for the fix. No data-integrity or blocking-path issues were found.

No files require special attention beyond the noted residual TOCTOU window in watcher.go.

Important Files Changed

Filename	Overview
internal/reporting/watcher.go	Adds clearActivityState before UpdateMessage in the terminal path and introduces a re-check guard in UpdateActivityIndicator; a narrow TOCTOU window between the re-check and the API call remains.
internal/reporting/watcher_test.go	Adds TestSlackTaskReporter_ActivitySkipsUpdateWhenStateCleared to verify zero Slack API calls when state is pre-cleared; deterministic test for the fix, coverage is appropriate for a unit-test suite.

Sequence Diagram

sequenceDiagram
    participant A as ActivityGoroutine
    participant T as TerminalPath
    participant S as SlackAPI

    Note over A,S: Happy path after fix
    A->>A: lock, read messageTS, unlock
    A->>A: build msg
    T->>T: clearActivityState
    T->>S: UpdateMessage with final content
    A->>A: re-check, state nil, bail out

    Note over A,S: Residual narrow race
    A->>A: lock, read messageTS, unlock
    A->>A: build msg
    A->>A: re-check passes, unlock
    T->>T: clearActivityState
    T->>S: UpdateMessage with final content
    A->>S: UpdateMessage with activity, may overwrite

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: internal/reporting/watcher.go
Line: 566-573

Comment:
**Residual TOCTOU window between re-check and API call**

After the re-check mutex is released (line 571) and before `UpdateMessage` is called (line 573), the terminal path can still: (1) `clearActivityState`, then (2) complete its own `UpdateMessage` with the final content — leaving the activity goroutine's subsequent `UpdateMessage` to overwrite the final message.

```
Activity:  lock → check passes → unlock → [window] → UpdateMessage(activity) ← last-write wins
Terminal:                                   clearState → UpdateMessage(final)
```

Because both calls are Slack network round-trips (~100 ms each), the window is narrow but real. The fix meaningfully narrows it (the earlier gap between the first mutex drop at line 548 and the API call was much wider), so this is an improvement — but the residual TOCTOU is inherent to the check-then-act pattern across an unlocked API call. If this bites in practice, one mitigation would be a monotonic generation counter on `activityState` that the activity goroutine captures and re-validates just before the API call, though that adds complexity for a cosmetic concern.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "fix(slack): prevent activity indicator r..." | Re-trigger Greptile}

[greptile-apps]

Residual TOCTOU window between re-check and API call

After the re-check mutex is released (line 571) and before UpdateMessage is called (line 573), the terminal path can still: (1) clearActivityState, then (2) complete its own UpdateMessage with the final content — leaving the activity goroutine's subsequent UpdateMessage to overwrite the final message.

Activity:  lock → check passes → unlock → [window] → UpdateMessage(activity) ← last-write wins
Terminal:                                   clearState → UpdateMessage(final)

Because both calls are Slack network round-trips (~100 ms each), the window is narrow but real. The fix meaningfully narrows it (the earlier gap between the first mutex drop at line 548 and the API call was much wider), so this is an improvement — but the residual TOCTOU is inherent to the check-then-act pattern across an unlocked API call. If this bites in practice, one mitigation would be a monotonic generation counter on activityState that the activity goroutine captures and re-validates just before the API call, though that adds complexity for a cosmetic concern.

Prompt To Fix With AI

This is a comment left during a code review.
Path: internal/reporting/watcher.go
Line: 566-573

Comment:
**Residual TOCTOU window between re-check and API call**

After the re-check mutex is released (line 571) and before `UpdateMessage` is called (line 573), the terminal path can still: (1) `clearActivityState`, then (2) complete its own `UpdateMessage` with the final content — leaving the activity goroutine's subsequent `UpdateMessage` to overwrite the final message.

```
Activity:  lock → check passes → unlock → [window] → UpdateMessage(activity) ← last-write wins
Terminal:                                   clearState → UpdateMessage(final)
```

Because both calls are Slack network round-trips (~100 ms each), the window is narrow but real. The fix meaningfully narrows it (the earlier gap between the first mutex drop at line 548 and the API call was much wider), so this is an improvement — but the residual TOCTOU is inherent to the check-then-act pattern across an unlocked API call. If this bites in practice, one mitigation would be a monotonic generation counter on `activityState` that the activity goroutine captures and re-validates just before the API call, though that adds complexity for a cosmetic concern.

How can I resolve this? If you propose a fix, please make it concise.

[anomalogravity]

Risk Assessment: Low

• Small, focused diff: 45 additions, 1 deletion across 2 files (one source, one test)
• No critical paths touched: Changes are confined to Slack reporting logic — no DB connectors, migrations, public API, ML algorithms, auth, or infrastructure
• Race condition fix: Reorders an existing call to run before (rather than after), and adds a mutex-guarded re-check in before the API call — both are minimal, surgical changes
• Test coverage: New test directly verifies the fix, using established test patterns in the file
• Greptile confidence: 5/5: Greptile rated this safe to merge. The only finding (P2) is a residual TOCTOU window inherent to the unlocked-API-call pattern — acknowledged in the PR description and consistent with existing codebase patterns
• No security, privacy, or external service implications: Pure internal concurrency fix for a cosmetic Slack message ordering issue

[anomalogravity]

Auto-approved: PR assessed as low risk by Gravity.

[kristiancailer]

@claude, not sure if Greptile's comment requires addressing but please take a look.