Skip to content

test(KEN-3997): wait for the Slack hold resume acknowledgement - #4982

Merged
vanillagreen-fleet-lanes[bot] merged 2 commits into
mainfrom
ken-3997
Oct 13, 2026
Merged

vanillagreen-fleet-lanes[bot] merged 2 commits into
mainfrom
ken-3997

Conversation

@vanillagreen-fleet-lanes

@vanillagreen-fleet-lanes vanillagreen-fleet-lanes Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • The resume rows wait for a later successful poll whose hold has ended. A refused poll can write its completion time after the setting was removed.
  • The existing live settings fixture pauses that refused poll before its status write. Each normal run therefore exercises the failure order seen on macOS.

Done when

  • Box 1: skills/slack/tests/hold.test.sh owns the resume wait and the deterministic fixture. The accepted timing loop passed 20 consecutive rounds. The cause and logs are attached to KEN-3997.
  • Box 2: The overseer reads the next macOS Slack Skill Tests runs after merge. This box remains open until that live sample passes.

Validation

  • The changed hold suite passed all 262 existing rows and controls in 222 seconds.
  • A disposable copy with the old timestamp-only wait failed both original resume assertions in each real settings variant: 53 passed, 8 failed. The failures match the macOS values.
  • The accepted timing loop passed 20 consecutive rounds: 1220 passed, 0 failed. It forces the same poll order on Linux. It does not measure native macOS filesystem behavior.
  • Native macOS CI passed all 262 hold rows in 290 seconds in run 38428595106.
  • Preflight, document limits, commit guards and push guards passed. The required CI wait returned complete/pass on this head. Copilot approved this head.

Merge decision

Head: a9c29f9

merge-route: queue cause=queue-occupied

Closes KEN-3997

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The synchronization change lacks a deterministic must-fail control proving the former race is detected.

1 open finding
What changed in this PR

Improves Slack hold tests by waiting for confirmed resume state before asserting notices.

Changes:

  • Extends poll waiting with successful, unheld-state acknowledgement.
  • Uses the stronger wait after removing hold settings.
File Description
skills/​slack/​tests/​hold.test.sh Prevents premature resume assertions.

🧠 Review effort: Balanced


💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread skills/slack/tests/hold.test.sh
@vanillagreen-fleet-lanes

Copy link
Copy Markdown
Contributor Author

The corrected resume wait passed 20 consecutive timing rounds on commit 39355ad. The loop reported 1220 passed and 0 failed in 638.713 seconds. Every round ran the TOML, local, private and named variants, plus caller-empty and the no-read control. Every real variant passed both original resume assertions and the later no-repeat assertion.

This local Linux loop forces the same poll ordering that makes the macOS rows fail. A barrier pauses the real relay's refused poll before the status write. The test removes or empties the setting, then releases that earlier poll. The old wait accepted its newer failed/held status and failed both original assertions. The corrected wait rejects that status and waits for the later successful/unheld poll. This proof measures the ordering, not a native macOS run or filesystem behavior.

The ordinary hold suite also passed all 262 existing rows and controls in 200 seconds. No suite or job bound changed. The cause and before-fix evidence are in the prior cause comment on this item.

Copilot requested this barrier in the shipped fixture. The overseer accepted that correction and the small continuation on ask 1791907066-135607-10529. The accepted timing proof carries forward. Post-merge box 2 stays open for live macOS load across the next Skill Tests runs.

@vanillagreen-fleet-lanes
vanillagreen-fleet-lanes Bot marked this pull request as draft October 13, 2026 16:03
A refused poll can read an invalid setting before removal and write its status afterward. Pause that real poll in each live-settings relay copy so the existing resume assertions must wait for successful release. Keep every existing row, control and bound. A disposable timestamp-only mutation of this fixture failed both resume assertions for all four settings variants: 53 passed, 8 failed. The corrected whole hold suite passed 262 rows in 222 seconds. Evidence: tmp/waiter.ttdLWh/old-wait.log and tmp/waiter.aBbd2w/hold.log.
@vanillagreen-fleet-lanes

Copy link
Copy Markdown
Contributor Author

Dispositions at 39355ad

review:5506355939 Fixed in a9c29f9: the normal live-setting fixture now forces the refused status write after removal. The old wait fails both existing resume assertions for every real settings variant. The corrected suite passes all 262 rows. The control and timing evidence are on KEN-3997.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approved

The deterministic fixture addresses the prior coverage concern without introducing unresolved issues.

0 open findings

🧠 Review effort: Balanced

@vanillagreen-fleet-lanes
vanillagreen-fleet-lanes Bot added this pull request to the merge queue Oct 13, 2026
Merged via the queue into main with commit ff0ace4 Oct 13, 2026
44 checks passed
@vanillagreen-fleet-lanes
vanillagreen-fleet-lanes Bot deleted the ken-3997 branch October 13, 2026 20:07
@vanillagreen-fleet-lanes

Copy link
Copy Markdown
Contributor Author

Merge close-out

  • PR test(KEN-3997): wait for the Slack hold resume acknowledgement #4982 merged through the queue at 2026-10-13T20:07:03Z as ff0ace4.
  • Cause: a refused poll read the invalid setting before removal. It wrote its completion timestamp after removal. The old timestamp-only wait accepted that refused poll, so the resume assertions could read an active hold and no delivered notices.
  • The resume rows now wait for a successful poll with no hold. The live fixture forces the refused poll to finish after setting removal. This makes the ordering repeatable.
  • Only skills/slack/tests/hold.test.sh changed. Every row and control remains. The relay source, suite bounds, job bounds and skill versions are unchanged.

Validation and review

  • The macOS-equivalent ordering loop passed 20 consecutive rounds: 1220 passed, 0 failed. The cause and loop proof are attached to KEN-3997.
  • The native macOS job in run 38428595106 reported suite=hold.test seconds=290 pass=262 fail=0. Its log is attached to KEN-3997.
  • The shipped fixture with the old timestamp-only wait failed both resume assertions across all real-setting variants: 53 passed, 8 failed.
  • Required CI passed. The error, test and external reviews passed. Copilot approved a9c29f9 in review 5506741781.
  • Both commits passed the commit guards before push.

Post-merge handoff

  • KEN-3997 is Verifying. Completion used --post-merge-at 2026-10-13T20:07:03Z --done-when-met 1. Box 1 is checked. Box 2 remains open.
  • The overseer owns box 2: inspect the next 10 Skill Tests runs on main or in the merge group that select the Slack shard on macOS. Record each job's suite=hold.test line on KEN-3997. These runs must show no hold.test failure.
  • The merge-triggered deadline is 2026-10-16T20:07:03Z. Pre-merge test runs do not count for box 2.
  • KEN-3997 has no downstream blocked issues. No downstream issue comment is due.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant