provision-roles.sql stops at its role-collision guard, and the collector toggle verbs exit 2 for a store problem (#4746, #4744)

[erikdarlingdata]

Fixes #4746
Fixes #4744

Why

#4746. Darling/tools/provision-roles.sql had no ON_ERROR_STOP, and psql carries on after an error by default. The role-collision guard's RAISE failed only its own DO block. So when a role named admin, viewer or mcp already existed without Darling's darling-managed marker, the ALTER ROLE ... PASSWORD lines right after the guard reset that role's password, and every grant after them ran too. A comment above the temp_file_limit lines also said a failure there stops the script. It does not: psql carries on.

Around that stop:

• The guard has to stop the run without overriding the caller. Switching the variable off again after the guard (\set ON_ERROR_STOP off) would undo a caller's -v ON_ERROR_STOP=1 or a psqlrc setting: the script would go on through every later error and exit 0, where a caller who asked for a stop expects exit 3. So the script saves the caller's value and puts it back.
• Line-order regexes cannot show that psql stops. The Npgsql live test drops the \set lines and would stop at the RAISE without them, so nothing ran the script through psql.
• The README's psql command did not pass -X, so the operator's psqlrc applied. With \set AUTOCOMMIT off in it, psql opens a transaction before the first statement and never commits it: psql exits 0 and every role, grant and setting rolls back.

#4744. --enable-collector and --disable-collector returned 1 for every failure (12 return 1 lines in ToggleCollectorAsync), so a script could not tell bad arguments from a store that is down. --check-settings already separates its failure kinds with CheckSettingsExitCode. Also, a postgres.connectionString that Npgsql cannot parse (for example sslmode=NotARealSslMode), or a managed credential file that cannot be read or unprotected, threw out of the connection build in both verbs and in --check-settings, and the process ended with an unhandled exception instead of an exit code.

What changes

Darling/tools/provision-roles.sql

• \set darling_saved_stop :ON_ERROR_STOP saves the caller's value just before the guard, \set ON_ERROR_STOP on sits right before step 1's guard, and \set ON_ERROR_STOP :darling_saved_stop puts the caller's value back right after it, before the first ALTER ROLE ... PASSWORD. Only the guard stops the run when the caller asked for nothing (psql exits with status 3); a caller who passed -v ON_ERROR_STOP=1 keeps it for the whole script. From psql 10, an unset ON_ERROR_STOP reads as off, so the saved value is always on or off. In psql 9.6 the unset variable stays the literal :ON_ERROR_STOP and reads as on, so the run would stop at its first error and an owner without superuser would get no grants (which contradicts the README). The script header, the comment at the guard and the README now say psql 10 or later. Step 0's SET before the guard and the temp_file_limit and slow-statement lines after it keep the caller's setting (psql's default is to carry on), so an owner without superuser still gets every grant it can make. A global -v ON_ERROR_STOP=1 in the README's command would have stopped that owner at the temp_file_limit line, before any grant, so the README's command does not get one.
• The script holds one DO block with the three checks in it (not three blocks), so the save and restore bracket that one block.
• The temp_file_limit comment now says a failure there does not stop the script, and what to do about it.
• The command in the header comment passes -X, with one line saying why.

Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs

• New CollectorToggleExitCode: Success = 0, UsageOrConfig = 1, StoreUnavailable = 2.
• Each of the 12 return 1 lines in ToggleCollectorAsync now returns the constant for its kind. Nine are UsageOrConfig: bad arguments, an unknown collector, a config that will not load, no postgres section, a managed store on a machine that is not Windows, a managed store whose credential is not stored, an empty connection string, a --server that names no server, and a --server that names more than one. Three are StoreUnavailable: the servers registry cannot be read, the write fails, and the read-back after the write fails. The success path returns Success. The doc comment names the codes.
• One shared method, TryBuildStoreConnectionString, builds the store connection string for every verb that opens the store, and holds the only catch (Exception ex) when (ex is not OperationCanceledException) for it. A managed store's credential read (TryBuildConnectionStringFromStoredCredential) and, for a bring-your-own string, EnsureStoreSearchPath (the collector toggles ask for it) run inside it, and a non-blank string is proved by building it the way the verb is about to (time zone pin and application name), so the verb's own build a few lines later cannot throw on it. It returns the message to print; the sentence postgres.connectionString could not be used: <reason> is written in that one method and nowhere else.
• Every verb calls it: --validate-config, --check-settings, the four endpoint verbs (--enable-mcp, --disable-mcp, --enable-web, --disable-web), --collapse-legacy-slices, --recompress-plan-dim, --add-server, --backfill-rollups, --enable-collector and --disable-collector. Each prints the message and exits 1; --check-settings returns CheckSettingsExitCode.ConfigError, which is also 1. The catches ToggleCollectorAsync and CheckSettingsAsync carried are gone. The other verbs let the exception escape when the connection string could not be parsed or the managed credential file could not be read. --validate-config still falls back to darling.json's own server list for a store that is merely unreachable; only a store setting that cannot be used at all exits 1. Each verb still builds its own connection with the pin and name on the constructor call, as the census tests require, and keeps its own wording for an empty string or a credential that was never stored.
• A managed store's unreadable credential is described as that: The stored store credential could not be read: <detail>, not a message that blames postgres.connectionString. A DPAPI failure (CryptographicException) gets its own explanation, DarlingSecrets.DescribeStoreCredentialDecryptFailure, instead of the raw "Key not valid for use in specified state". It names the store's credential file (pg-credential.dpapi), says only the machine that wrote it can read it, and does not offer the fixes meant for a monitored server's saved password (--add-server, --encrypt-password) that DescribeDecryptFailure gives. DescribeDecryptFailure itself is unchanged; anything else (a file that is not base64, a permission error) keeps its own message. The --configure-firewall note that already said the stored store credential could not be read (...) takes the same detail, so the two cannot drift. That note printed ex.Message before; it did not call DescribeDecryptFailure.

Darling/Darling.Tests

• ComposeStoreRolesLiveTests.cs: the script is still fed to Npgsql through WithoutPsqlMetaCommands (now three backslash lines), and two gated tests run the runtime's own psql.exe -X -f on the shipped script. One plants an unmarked viewer login with a known password and asserts psql exits 3, no admin or mcp role exists, and viewer's rolpassword and marker are unchanged. The other runs it against the cluster's maintenance database, where the grants fail, and asserts exit 0 on a plain run and exit 3 with -v ON_ERROR_STOP=1.
• ProvisionRolesCollisionGuardTests.cs: the stop pin follows the restore line; new pins for the save before the guard, the restore right after it and no line that forces the variable off; both shown psql commands pass -X; the README says the check tests the marker alone.
• DarlingCollectorToggleExitCodeTests.cs: a connection string that does not parse exits 1 for both verbs and for --check-settings; a managed credential file that cannot be unprotected does the same (Windows only) and now asserts the managed wording; the README pin also holds that a refused login exits 2 and which case differs between --check-settings and the toggles; two new tests show that an empty connection string is 1 from both and a managed credential that was never stored is 2 from --check-settings and 1 from the toggles.
• New DarlingCliUnusableStoreConnectionTests.cs: one case per verb that opens the store, for a bring-your-own string with sslmode=NotARealSslMode (8 verbs; the four endpoint verbs stop earlier, at the managed-mode guard), for a managed credential file that is not base64 (all 12 verbs, Windows only), and for a well-formed blob that DPAPI cannot unprotect (all 12, Windows only, asserts the DPAPI wording and no raw "Key not valid"). Each expects exit 1 and no exception. A wiring pin holds that the sentence appears once, that 8 call sites use the shared method, and that the stored credential is read bare in only two places (the shared method, and the firewall verb's best-effort note).
• ProvisionRolesCollisionGuardTests.cs: the comment that repeated the psql claim now says psql 10; a new pin holds that the script and the README say psql 10 or later and that the old sentence is gone.

Darling/README.md

• The paragraph after the psql command line says what happens on a name collision (the script stops before it changes that role, which lacks Darling's darling-managed marker) and that any other error is printed while the run carries on. The command passes -X, with a sentence on why: an AUTOCOMMIT off or ON_ERROR_STOP in the operator's psqlrc would change the run.
• The --enable-collector / --disable-collector codes are listed next to the --check-settings codes, and the verbs' own section says which failure is which, including that a refused login exits 2 (a wrong password is the store's answer, 28P01, pinned by a test) and that a connection string that cannot be used exits 1.
• It no longer says the two verbs separate failures "the same way". --check-settings exits 1 for an unusable store setting and for an empty connection string (config validation rejects it first) and 2 for a managed credential that was never stored; the toggles exit 1 for that last case. A pin holds the sentence and two tests hold the codes. A line lists the other verbs that open the store and says they print the problem and exit 1.
• The psql command is followed by "Use psql 10 or later", with the reason.

Test plan

Each issue's first commit holds only its tests. Run on those sources (dev's code):

• ProvisionRolesCollisionGuardTests: 2 of 3 fail (the order of the stop around the guard, and the false comment). The third, which pins that the README's psql line has no global stop, passes before and after.
• DarlingCollectorToggleExitCodeTests: 6 of 18 fail with Expected: 2, Actual: 1 (an unreachable store, an unreachable store while resolving --server, and a store that answers the login with an error, each for both verbs). The other 12 cases already returned 1 and are pins.
• The psql run of the shipped script, RED then green. RED on dev's script (the built copy of provision-roles.sql replaced by origin/dev's, not committed): TheBringYourOwnScript_RunByPsql_StopsAtAnUnmarkedRole_AndChangesNothing_Gated [FAIL], psql exited 0, not 3: the script did not stop at the unmarked viewer role. RED on a script that ends the guard with \set ON_ERROR_STOP off: TheBringYourOwnScript_RunByPsql_KeepsTheCallersOnErrorStop_Gated [FAIL], psql exited 0, not 3, with -v ON_ERROR_STOP=1: the script switched the caller's stop off. Both pass on the fixed script.
• With DARLING_TEST_PGRUNTIME set to an extracted runtime: ComposeStoreRolesLiveTests and ProvisionRolesCollisionGuardTests together, Total 16, Failed 0, Skipped 0 (so the two new psql tests ran, and the existing bring-your-own live test ran on the script with three backslash lines dropped).
• After the fixes DarlingCollectorToggleExitCodeTests passes (27 tests) and ProvisionRolesCollisionGuardTests passes (8).
• Darling.Tests builds with 0 warnings.
• Targeted classes pass: everything that names EnsureStoreSearchPath, CheckSettings or provision-roles, every class in a test file that mentions the README, and DocCommentHygieneTests (60 classes, Total 1218, Failed 0, Skipped 24 live tests without a database).
• The connection-string census tests pass (StoreSessionTimeZonePinCensusTests, StoreApplicationNameCensusTests): they want the time zone pin and application name on the connection-constructing call itself, which is why each verb still builds its own connection and the shared method only proves the string first. The first version of the shared method had the text of a constructor call inside a doc comment, which the census reads as a call site, and both census tests failed with "unterminated call"; the comment is reworded.
• Full Darling.Tests run once on the final tree (with origin/dev merged), no database and no runtime configured: Total 17831, Failed 0, Skipped 1162 (live tests), Not Run 1 (the one Explicit test). Build: 0 warnings.
• Every verb that opens the store now exits 1 on an unusable store setting. RED first, on the sources of the tests-only commit: DarlingCliUnusableStoreConnectionTests Total 21, Failed 15. Nine System.FormatException : The input is not a valid Base-64 string (the managed credential file: --validate-config, the four endpoint verbs, --collapse-legacy-slices, --recompress-plan-dim, --add-server, --backfill-rollups); four System.ArgumentException : Couldn't set sslmode (Parameter 'sslmode') (--validate-config, --collapse-legacy-slices, --recompress-plan-dim, --backfill-rollups); --add-server with a bad string Assert.Contains() Failure: Sub-string not found (it printed "Could not reach the store"); and the wiring pin Assert.Single() Failure: The collection contained 3 items (the sentence was written three times). --check-settings, --enable-collector and --disable-collector passed before and after (6 cases). After the fix: Total 46 with DarlingCollectorToggleExitCodeTests, Failed 0.
• The managed wording. RED on the sources without it, tests updated: DarlingCliUnusableStoreConnectionTests and DarlingCollectorToggleExitCodeTests Total 58, Failed 27 (24 new cases, all 12 verbs for each of the two credential kinds, plus the 3 existing tests that asserted the old text). After: Failed 0. The test seam for an unreadable managed credential already existed (a file at CredentialPathFor(dataDirectory)), so both credential kinds are covered.
• The store credential's own DPAPI text. RED on the sources without it: DarlingCliUnusableStoreConnectionTests Total 33, Failed 12, each Not found: "pg-credential.dpapi". After: DarlingCliUnusableStoreConnectionTests, DarlingCollectorToggleExitCodeTests, DarlingSecretsDecryptFailureTests and DocCommentHygieneTests Total 143, Failed 0. Full Darling.Tests once more: Total 17832, Failed 0, Skipped 1162.
• Not run against the old sources: TheScriptAndTheReadme_SayPsql10OrLater, the README pin for the differing exit codes, and the two exit-code tests for --check-settings (they assert text or codes that the old sources already had or lacked by construction). I did not run a psql 9.6 to confirm the version claim (it is the one in provision-roles.sql keeps going after an error, including past its role-collision guard #4746).
• Not run: the new toggle and --check-settings tests against the old code (reverting DarlingCliCommands.cs); the crash they pin comes from EnsureStoreSearchPath and the credential read, which had no catch.
• Not covered: a store that accepts the login and then refuses the write. No test double can do that without a live store. It reaches the same catch as the two covered store cases (a closed port, and a listener that answers the login with a 28P01 error).

CHANGELOG

SECTION: None
ENTRY:

• provision-roles.sql stops when a role it would create already exists without Darling's darling-managed marker ([provision-roles.sql stops at its role-collision guard, and the collector toggle verbs exit 2 for a store problem (#4746, #4744) #4809]) - The guard raised an error, but psql carried on and reset that role's password and granted it access. Only the guard stops the run, so an owner without superuser still gets every grant it can make, and a caller's own ON_ERROR_STOP (psql -v ON_ERROR_STOP=1) is kept after the guard. That needs psql 10 or later (psql 9.6 stops the run at its first error), which the script and the README now say. The documented psql command now passes -X, so an AUTOCOMMIT off in the operator's psqlrc cannot leave every role and grant uncommitted.
• --enable-collector and --disable-collector tell a store problem from a usage problem ([provision-roles.sql stops at its role-collision guard, and the collector toggle verbs exit 2 for a store problem (#4746, #4744) #4809]) - They exit 2 when the store cannot be reached, refuses the login, or refuses the change, and 1 for bad arguments or configuration. Before, every failure exited 1. A connection string that cannot be parsed, or a managed credential file that cannot be read, now prints an error and exits 1 instead of ending in an unhandled exception, and so do --check-settings (its config exit code, 1), --validate-config, --enable-mcp, --disable-mcp, --enable-web, --disable-web, --collapse-legacy-slices, --recompress-plan-dim, --add-server and --backfill-rollups. For a managed store the message says the stored store credential could not be read, names the store's credential file and says only the machine that wrote it can read it, instead of blaming postgres.connectionString. The README now says where --check-settings and the collector verbs differ: a managed credential that was never stored is 2 from --check-settings and 1 from the collector verbs.
  REF:
  [provision-roles.sql stops at its role-collision guard, and the collector toggle verbs exit 2 for a store problem (#4746, #4744) #4809]: provision-roles.sql stops at its role-collision guard, and the collector toggle verbs exit 2 for a store problem (#4746, #4744) #4809