fix(admin): decide a station delete's refusal under the lock, before revoking #40

2026-11-11T23:24:39Z

james.maes commented

2026-11-11 23:24:39 +00:00

Fixes the "should fix" finding of the independent review of PR 39, and adds the tests that review found missing. No wire, contract or schema change.

The defect

PrintingService.deleteDevice decided "this station is connected and printing" from a plan read with no lock, then revoked the station, and only then read the plan again under PrintingAdmission.lock and FOR UPDATE. A revoked station never counts as connected, so that second read could not refuse. A station that was idle at the first read and claimed a job before the revocation was deleted, and the job was closed as OUTCOME_UNKNOWN although the station had the content and could print it.

The fix

The delete is one transaction on one connection:

tenant admission lock, then the station's row lock (FOR UPDATE), the existing order;
the plan is read and the refusal decided there;
the REVOKE control runs on the same connection (PrintingDeviceControls.action(Connection, ...), the existing code split from its transaction wrapper);
jobs are closed, the deletion is recorded, the rows are deleted, one commit.

A claim, a job event and a heartbeat take the station's row lock before they change anything. So a claim either commits before the delete locks, and the delete sees the claimed job and is refused, or it waits for the delete and is answered 404 Device not found. Nothing can land between the decision and the revocation.

Two consequences:

A refused or failed delete changes nothing. Before, a delete whose transaction failed left the station revoked.
The revocation of a delete no longer goes through the public PrintingService.deviceAction, and the station's connection is woken once, after the commit. A host that wraps deviceAction to observe revocations would not see this one. I know of no such host; say if that matters.

deletePrinter did not have the window: it decides under the station's row lock and revokes nothing. It is unchanged, with tests added.

Also in this change

HTTP API text restored. RC.3 changed a text that an HTTP route returns: PUT /printing/v1/admin/devices/{deviceId}/printers/{printerId} for a removed printer. It answers "Printer was removed; share it from the device again before changing its approval" again, so the CHANGELOG's "HTTP API texts are unchanged" holds. The other changed text ("Revoke the print station's access before removing it") is raised only by removeDevice, which no HTTP or agent route calls; it stays.
Startup backfill. initialize() fills device_name / printer_name only on jobs whose station or printer still exists (AND EXISTS (...)), so jobs of a deleted station are no longer rewritten with the same nulls on every start. They are still read on every start: the name columns have no index. That part is not changed here.
CHANGELOG.md: new 1.2.0-RC.4 entry, and the RC.3 sentence about the revocation being a separate step is corrected.

Tests added (`PrintingDeleteTest`, 7 to 18)

A station claims a job after everything the delete reads without a lock and immediately before its first row lock: refused, job and station untouched. A second service on a proxied data source runs the claim at that exact point, so there is no timing in it. The same for a printer.
A claim in flight holds the station's row lock with its job not yet committed: the delete is observed waiting for that lock, then refuses once the claim commits.
A printer that is printing on a connected station is refused by the service, the review and the final step; another printer of the selection is still deleted.
An already revoked station is deleted without a second revocation; a delete whose transaction fails changes nothing and is retried.
A failed delete of a connected station leaves it with its access, printer and queued job; the final step reports that nothing was changed; a foreign-key violation (23503) the pre-check does not see is named as the missing schema step.
The final step refuses a station that started printing after the review.
The review and the final step report a record that another administrator deleted meanwhile, and a plan refused for another reason.
An aggregate over print jobs counts only the caller's tenant, for both tenants.
The restored HTTP API text.

Run against PrintingService as it is on main, four of the new tests fail as intended: the two station race tests (the delete went through), the failed-delete test (the station was left revoked) and the restored text.

Not done

No PostgreSQL run. The POM has no PostgreSQL driver and no Testcontainers, and the instruction was not to add build dependencies. The delete, the lock wait and the schema step are exercised on the suite's in-memory H2 only. Row locks and read-committed visibility after a lock wait are what the fix relies on; PostgreSQL provides both, but that is not executed here.

Verification

./mvnw -B -Pui-demo clean verify on b063f9a, nothing skipped: BUILD SUCCESS, 227 tests, 0 failures, 0 errors, 1 skipped (PrintingWorkloadTest, opt-in, as on main), 0 Checkstyle violations. CI runs mvn -T1C verify with -Pui-demo for this branch (.gitea/workflows/build.yaml and the shared maven.yaml). Locally the registry was supplied through a settings file with the public QRun-IO Gitea Maven repository, as CI's own settings file does.

🤖 Generated with Claude Code

Fixes the "should fix" finding of the independent review of PR 39, and adds the tests that review found missing. No wire, contract or schema change. ## The defect `PrintingService.deleteDevice` decided "this station is connected and printing" from a plan read with no lock, then revoked the station, and only then read the plan again under `PrintingAdmission.lock` and `FOR UPDATE`. A revoked station never counts as connected, so that second read could not refuse. A station that was idle at the first read and claimed a job before the revocation was deleted, and the job was closed as `OUTCOME_UNKNOWN` although the station had the content and could print it. ## The fix The delete is one transaction on one connection: 1. tenant admission lock, then the station's row lock (`FOR UPDATE`), the existing order; 2. the plan is read and the refusal decided there; 3. the REVOKE control runs on the same connection (`PrintingDeviceControls.action(Connection, ...)`, the existing code split from its transaction wrapper); 4. jobs are closed, the deletion is recorded, the rows are deleted, one commit. A claim, a job event and a heartbeat take the station's row lock before they change anything. So a claim either commits before the delete locks, and the delete sees the claimed job and is refused, or it waits for the delete and is answered `404 Device not found`. Nothing can land between the decision and the revocation. Two consequences: - A refused or failed delete changes nothing. Before, a delete whose transaction failed left the station revoked. - The revocation of a delete no longer goes through the public `PrintingService.deviceAction`, and the station's connection is woken once, after the commit. A host that wraps `deviceAction` to observe revocations would not see this one. I know of no such host; say if that matters. `deletePrinter` did not have the window: it decides under the station's row lock and revokes nothing. It is unchanged, with tests added. ## Also in this change - **HTTP API text restored.** RC.3 changed a text that an HTTP route returns: `PUT /printing/v1/admin/devices/{deviceId}/printers/{printerId}` for a removed printer. It answers "Printer was removed; share it from the device again before changing its approval" again, so the CHANGELOG's "HTTP API texts are unchanged" holds. The other changed text ("Revoke the print station's access before removing it") is raised only by `removeDevice`, which no HTTP or agent route calls; it stays. - **Startup backfill.** `initialize()` fills `device_name` / `printer_name` only on jobs whose station or printer still exists (`AND EXISTS (...)`), so jobs of a deleted station are no longer rewritten with the same nulls on every start. They are still read on every start: the name columns have no index. That part is not changed here. - `CHANGELOG.md`: new `1.2.0-RC.4` entry, and the RC.3 sentence about the revocation being a separate step is corrected. ## Tests added (`PrintingDeleteTest`, 7 to 18) - A station claims a job after everything the delete reads without a lock and immediately before its first row lock: refused, job and station untouched. A second service on a proxied data source runs the claim at that exact point, so there is no timing in it. The same for a printer. - A claim in flight holds the station's row lock with its job not yet committed: the delete is observed waiting for that lock, then refuses once the claim commits. - A printer that is printing on a connected station is refused by the service, the review and the final step; another printer of the selection is still deleted. - An already revoked station is deleted without a second revocation; a delete whose transaction fails changes nothing and is retried. - A failed delete of a connected station leaves it with its access, printer and queued job; the final step reports that nothing was changed; a foreign-key violation (`23503`) the pre-check does not see is named as the missing schema step. - The final step refuses a station that started printing after the review. - The review and the final step report a record that another administrator deleted meanwhile, and a plan refused for another reason. - An aggregate over print jobs counts only the caller's tenant, for both tenants. - The restored HTTP API text. Run against `PrintingService` as it is on `main`, four of the new tests fail as intended: the two station race tests (the delete went through), the failed-delete test (the station was left revoked) and the restored text. ## Not done - **No PostgreSQL run.** The POM has no PostgreSQL driver and no Testcontainers, and the instruction was not to add build dependencies. The delete, the lock wait and the schema step are exercised on the suite's in-memory H2 only. Row locks and read-committed visibility after a lock wait are what the fix relies on; PostgreSQL provides both, but that is not executed here. ## Verification `./mvnw -B -Pui-demo clean verify` on `b063f9a`, nothing skipped: BUILD SUCCESS, 227 tests, 0 failures, 0 errors, 1 skipped (`PrintingWorkloadTest`, opt-in, as on `main`), 0 Checkstyle violations. CI runs `mvn -T1C verify` with `-Pui-demo` for this branch (`.gitea/workflows/build.yaml` and the shared `maven.yaml`). Locally the registry was supplied through a settings file with the public QRun-IO Gitea Maven repository, as CI's own settings file does. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

james.maes added 1 commit 2026-11-11 23:24:39 +00:00

fix(admin): decide a station delete's refusal under the lock, before revoking

maven / build (push) Successful in 2m13s

Details

build / maven (push) Successful in 2m13s

Details

b063f9a55f

deleteDevice decided "connected and printing" from a read without a lock,
revoked the station, and only then locked and read again. A revoked station
never counts as connected, so the locked read could not refuse: a station
that claimed a job between the first read and the revocation was deleted and
the job closed as OUTCOME_UNKNOWN although the station had its content.

The delete is now one transaction: tenant admission lock, then the station's
row lock (the lock a claim, a job event and a heartbeat take first), the
refusal, the REVOKE control on the same connection, the job closing and the
row deletes, one commit. A refused or failed delete changes nothing; the
station is no longer left revoked when the delete's transaction fails.
PrintingDeviceControls.action gains an overload that runs inside the
caller's transaction. deletePrinter already decided under the station's row
lock and is unchanged.

Also: the printer approval route returns "Printer was removed; share it from
the device again before changing its approval" again (RC.3 changed an HTTP
API text); the startup name backfill only updates jobs whose station or
printer still exists.

Tests: the claim that lands just before the delete locks (station, printer),
the claim in flight that the delete waits for, a printer refused on a live
station, an already revoked station, rollback and retry of a failed
transaction, the 23503 mapping, the final step refusing after the review,
review plan errors, and a tenant-scoped aggregate over print jobs.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>

james.maes added 1 commit 2026-11-11 23:30:54 +00:00

fix(delete): write names only onto jobs that lack one; correct the changelog on the old backfill

maven / build (push) Successful in 2m22s

Details

build / maven (push) Successful in 2m22s

Details

d7c7c203c3

Review of PR 40: the name backfill at delete rewrote every job of the station
while holding the tenant's admission lock, and the changelog called the old
startup backfill a no-op where it nulled a queue name.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>

james.maes merged commit 5f0dbae0be into main

2026-11-12 15:11:15 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: QRun-IO/qbit-printing#40

fix(admin): decide a station delete's refusal under the lock, before revoking #40

The defect

The fix

Also in this change

Tests added (PrintingDeleteTest, 7 to 18)

Not done

Verification

Tests added (`PrintingDeleteTest`, 7 to 18)