[dlamd1dai]

Motivation

Follow-up to #13231, raised by Argus in its review there. Depends on #14321, which replaced #13231 with the same change on an upstream branch. This branch is stacked on #13231's commits from before the rebase, so only the last commit, fix(rccl): roll back a GIN activation that fails after connect, belongs here. I will rebase once #14321 merges.

#14321 sets devr->ginEnabled only after ncclGinConnectOnce succeeds. After that, ncclDevrCommCreateInternal still registers every existing memory with GIN and then updates every existing window's GIN handles. If anything fails in that stretch (a symMemoryRegisterGin call, ncclGinDevCommSetup, stream creation, or symWindowInitGin), GIN stays latched:

• The memory that failed to register has its ginSegmentInfos freed, and memories later in the list were never registered.
• Windows never got their GIN handles, because the window update runs after ncclGinDevCommSetup.
• The next create computes ginActivated false, skips both the registration and the window update, and still builds a full GIN devcomm. A later ncclCommWindowRegister on one of those memories then indexes the ginSegmentInfos that was freed.

Technical Details

• ncclDevrCommCreateInternal treats connect, registering the existing memories, and updating the existing windows as one activation. It records completion in ginActivationDone, set once the window update loop finishes.
• On any failure before that point:
  • symGinActivationRollbackWindows (on the fail_stream path, while the stream is still alive) frees the segment windows attached to existing windows, clears their GIN handles, and copies the cleared window to the device.
  • symGinActivationRollbackMemories (on the fail path) deregisters every memory holding ginSegmentInfos, frees them, resets numGinSegments to 1 as for memory obtained before GIN, and clears ginEnabled.
• Rolling back everything GIN holds is safe because nothing had GIN state before this activation. ginActivated is only true while ginEnabled was false, and ginSegmentInfos and a window's ginMultiSegmentWins are only set once GIN is enabled.
• ncclGinConnectOnce keeps connected true, so the retry does not reconnect. It re-registers the memories and updates the windows again.
• Failures after the window update are unchanged. GIN is then fully activated for every existing memory and window, and keeping it latched is correct.
• As with the connect retry in fix(rccl): latch GIN only after a successful connect #14321, the rollback assumes the failure happened on every rank.

Issue Tracking

JIRA ID: AICOMRCCL-2879

Test Plan

• New DevrCommCreateInternalTest.GinRegisterFailsAfterConnect_RollsBackActivation: connect succeeds, and two GIN-capable memories are on memHead. The first registers and the second fails. The test checks:
  • ginEnabled is already true while registration runs.
  • After the failure, the first memory is deregistered, neither memory keeps ginSegmentInfos, and ginEnabled is false.
  • A second create connects and registers again, then rolls back the same way.
• Full host-test pipeline (run_host_tests.sh deps through run) in rocm/dev-ubuntu-22.04:7.2.

Test Result

• rccl-UnitTestsMicro: 1664 of 1664 passed, and every other host binary passed.
• With the dev_runtime.cc rollback reverted, the new test fails on the ginEnabled check after the first create.
• With devr->ginEnabled = true; removed, the new test fails on the check inside the register hook.
• Not covered by host tests: the window rollback, and failures in ncclGinDevCommSetup or stream creation. The fixture cannot reach the devcomm build past the registration loop.

Submission Checklist

• Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.

[therock-pr-bot]

✅ All Policy Checks Passed

Check	Status	Details
📝 PR Description	✅ Pass	—
⛔ Forbidden Files	✅ Pass	—
🧪 Unit Test	⚠️ Warning	Error: Source/code files changed without an accompanying unit test. Expected: add at least one test file named like test_<name>.py / test_<name>.cpp (or <name>_test.*). Current: code file(s) changed: projects/rccl/src/dev_runtime.cc, projects/rccl/test/host/dev-runtime-test.cc, projects/rccl/test/host/fakes/dev_runtime_micro_fakes.cc, projects/rccl/test/host/fakes/dev_runtime_micro_fakes.h; no test file found
🚫 Draft PR	🔜 To Be Enabled	—
🚩 Feature Flag	🔜 To Be Enabled	—
📊 Code Coverage	🔜 To Be Enabled	—

🎉 All policy checks passed!

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

[amd-argus]

ARGUS

Findings

3 more finding(s), not posted inline

• projects/rccl/src/dev_runtime.cc:861 (Low) symGinActivationRollbackMemories is a third copy of the GIN teardown in symMemoryRegisterGin's fail path (793-801) and symMemoryUnregister (817-819); one shared helper would keep them in step.
• projects/rccl/test/host/dev-runtime-test.cc:5646 (Low) No assertion that the rollback clears mems[0].ginHostWins, which caches goodHostWin; add one.
• projects/rccl/test/host/dev-runtime-test.cc:5659 (Nit) Comment says a later valid create still connects, but the test runs one create; describe what it checks.

Bottom line: Worth settling before approval: the window rollback at dev_runtime.cc:2250 has no test; zeroing the requirement-builder fakes' output would let one reach it.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

[amd-argus]

Medium

symGinActivationRollbackWindows has no test, so the suite passes with this call removed. The fixture stops early because the requirement-builder fakes at dev_runtime_micro_fakes.cc:498 leave the stack ncclDevResourceRequirements uninitialised, and line 2109 writes through outBufferHandle. Could those fakes zero their output? A test could then fail g_devrAllocAndPopulateSegmentWindows on a second GIN window. The first window's hook must return a shadow-pool object, since the default nullptr (dev_runtime_micro_fakes.cc:398) is skipped at line 838.

[amd-argus]

Low

Could this header fit the one-line budget? The safety argument still fits:

Suggested change

	// now came from this activation.
	// Undo a partial GIN activation; memories and windows held no GIN state before it.

[dlamd1dai]

Closing in favor of #14325. It has the same commits (head ad48d9d) on an upstream branch, so the RCCL coco PR gate can run without a fork override.