Page MenuHomePhabricator

Fix a race between Harbormaster and reviewers (often bots) to publish drafts for review
ClosedPublic

Authored by epriestley on Jan 24 2018, 6:50 PM.
Tags
None
Referenced Files
F15531332: D18921.id.diff
Wed, Apr 23, 10:39 AM
F15527564: D18921.diff
Tue, Apr 22, 4:43 AM
F15486582: D18921.id45383.diff
Thu, Apr 10, 8:13 AM
F15438847: D18921.id45383.diff
Mar 26 2025, 4:00 AM
F15417045: D18921.diff
Mar 20 2025, 2:59 PM
F15376482: D18921.id45382.diff
Mar 13 2025, 3:57 AM
F15349520: D18921.id45384.diff
Mar 10 2025, 2:59 PM
F15335574: D18921.diff
Mar 8 2025, 4:33 PM
Subscribers
None

Details

Summary

See PHI309. There is a window of time between when all builds pass and when Harbormaster actually publishes a revision out of draft.

If any other user tries to interact with the revision during that window, they'll pick up the undraft transaction as a side effect. However, they won't have permission to apply it and will be stopped by a validation error.

Instead, only automatically publish a revision if the actor is the revision author or some system/application user (essentially always Harbormaster).

Test Plan
  • Added a echo ...; sleep(30); to HarbormasterBuildEngine->updateBuildable() before the applyTransactions() at the bottom.
  • Wrote an "Always, run an HTTP request" Herald rule and Harbormaster build plan.
  • Ran daemons with bin/phd debug task.
  • Created a new revision with arc diff, as user A.
  • Waited for phd to enter the race window.
  • In a separate browser, as user B, submitted a comment via differential.revision.edit.
    • Before patch: edits during the race window were rejected with a validation error, "you don't have permission to request review".
    • After patch: edits go through cleanly.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable