Skip to content

Send the isolation mutex name on the wire's IsolationMutexName field#427

Merged
ppXD merged 1 commit into
mainfrom
fix/isolation-mutex-name-wiring
Jun 10, 2026
Merged

Send the isolation mutex name on the wire's IsolationMutexName field#427
ppXD merged 1 commit into
mainfrom
fix/isolation-mutex-name-wiring

Conversation

@ppXD

@ppXD ppXD commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Bug: HalibutMachineExecutionStrategy (both dispatch sites) built the agent isolation mutex name but passed it into StartScriptCommand's taskId parameter (arg 7) instead of isolationMutexName (arg 5), leaving the wire field null. The agent (ScriptIsolationMutex) then fell back to its single "default" mutex — so the per-action name the code + ARCH.7 docs + tests claimed to send never reached the wire, and the SHA hash polluted the correlation field. Linux/WindowsTentacleUpgradeStrategy passed null + the ticket id the same way.
  • Decision (semantics): serialise FullIsolation scripts per machine — the safe default — made explicit via a shared ScriptIsolationMutexNames.ForMachine(machineId) used by both deployment dispatch and the upgrade strategies. So every FullIsolation script dispatched to a machine (deployment or upgrade) serialises behind one writer lock, instead of relying on the accidental null → "default" fallback. Crucially, deployments and upgrades now share the identical name, preserving the deploy⇄upgrade agent-level serialisation (they were both on "default" before; a one-sided change would have split them onto different mutexes).
  • Removed the per-action GenerateMutexName (keyed by serverTaskId, which actually defeated cross-deployment serialisation — FullIsolation's whole purpose). taskId now carries the server task id for correlation; ResolveParentTraceId ignores it, so this is behaviour-neutral on the agent.
  • Non-breaking: on a single-machine agent the global lock stays global — serialisation behaviour is unchanged. Only the wire field, the doc comments, and the tests now reflect it. No agent-side change required (it already reads IsolationMutexName, null-falling-back to "default"); old-server↔new-agent and new-server↔old-agent both still serialise.

Test plan

  • Unit (red→green): new ExecuteScriptAsync_PassesMachineScopedMutexNameAsIsolationMutexName_NotTaskId captures the dispatched StartScriptCommand and asserts IsolationMutexName == ForMachine(machineId) + TaskId == ServerTaskId — the wire field no prior test pinned, which is why the mis-wiring survived (confirmed RED before the fix)
  • Unit: ScriptIsolationMutexNamesTests pins the format literal + same-machine-same-name / different-machine-different-name (the shared-string invariant deploy⇄upgrade serialisation depends on)
  • Unit: both upgrade strategies' isolation-pin tests now assert IsolationMutexName == ForMachine(machineId)
  • Removed the stale per-action GenerateMutexName tests + ARCH.7 comments; corrected the ticket test to check ScriptTicket.TaskId (the actual Guid) not the SHA-in-TaskId
  • 251 affected tests green; broad DeploymentExecution + Deployments + Machines.Upgrade sweep 5017/5017 green; dotnet build Squid.sln 0 errors
  • Tentacle ScriptExecution suite: 30 LocalScriptServiceTests fail only on a near-full dev disk (DiskSpaceChecker 10% floor in StartScript — environmental, agent code untouched by this change, branch-independent; all 30 confirmed disk-space IOException, zero mutex-related; pass on CI)

@ppXD ppXD added this to the 1.8.17 milestone Jun 10, 2026
Script dispatch built the agent's isolation mutex name but passed it into
StartScriptCommand's taskId parameter (arg 7) instead of isolationMutexName
(arg 5), leaving the real field null. The agent then fell back to its single
"default" mutex, so the per-action name the code and tests claimed to send
never reached the wire and the SHA hash polluted the correlation field. The
two upgrade strategies passed null + the ticket id the same way.

Decide the semantics and make code, comments and tests agree: serialise
FullIsolation scripts per machine — the safe default — made explicit via a
shared ScriptIsolationMutexNames.ForMachine used by BOTH deployment dispatch
and the upgrade strategies, so every FullIsolation script to a machine
(deployment or upgrade) serialises behind one writer lock instead of relying
on the accidental null -> "default" fallback. Remove the per-action
GenerateMutexName (keyed by serverTaskId, which actually defeated cross-
deployment serialisation); taskId now carries the server task id for
correlation.

Behaviour on a single-machine agent is unchanged (the global lock stays
global), but the wire field, the doc comments and the tests now reflect it.
Add a test that captures the dispatched StartScriptCommand and asserts
IsolationMutexName — the field no prior test pinned, which is why the
mis-wiring survived.
@ppXD
ppXD force-pushed the fix/isolation-mutex-name-wiring branch from 223b532 to 77b132a Compare June 10, 2026 10:02
@ppXD
ppXD merged commit af3d913 into main Jun 10, 2026
14 of 15 checks passed
@ppXD
ppXD deleted the fix/isolation-mutex-name-wiring branch June 10, 2026 10:10
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