Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve continuous export failure recovery by moving the export operation "progress marker" from USB drive to VxScan store #4213

Merged
merged 5 commits into from
Nov 15, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions apps/central-scan/backend/src/store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,10 @@ function dateTimeFromNoOffsetSqliteDate(noOffsetSqliteDate: string): DateTime {
export class Store {
private constructor(private readonly client: DbClient) {}

// Used by shared CVR export logic in libs/backend
// eslint-disable-next-line vx/gts-no-public-class-fields
readonly scannerType = 'central';

getDbPath(): string {
return this.client.getDatabasePath();
}
Expand Down
10 changes: 8 additions & 2 deletions apps/scan/backend/schema.sql
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,19 @@ create table sheets (
);

create table system_settings (
-- enforce singleton table
-- Enforce singleton table
id integer primary key check (id = 1),
data text not null -- JSON blob
);

create table is_continuous_export_operation_in_progress (
-- Enforce singleton table
id integer primary key check (id = 1),
is_continuous_export_operation_in_progress boolean not null
);

create table export_directory_name (
-- enforce singleton table
-- Enforce singleton table
id integer primary key check (id = 1),
export_directory_name text not null
);
Expand Down
26 changes: 13 additions & 13 deletions apps/scan/backend/src/app_config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -164,9 +164,9 @@ test("if there's only one precinct in the election, it's selected automatically
test('continuous CVR export', async () => {
await withApp(
{},
async ({ apiClient, mockAuth, mockScanner, mockUsbDrive }) => {
async ({ apiClient, mockAuth, mockScanner, mockUsbDrive, workspace }) => {
await configureApp(apiClient, mockAuth, mockUsbDrive, { testMode: true });
await scanBallot(mockScanner, mockUsbDrive, apiClient, 0);
await scanBallot(mockScanner, apiClient, workspace.store, 0);

const exportDirectoryPaths = await getCastVoteRecordExportDirectoryPaths(
mockUsbDrive.usbDrive
Expand Down Expand Up @@ -216,10 +216,10 @@ test('continuous CVR export', async () => {
test('continuous CVR export, including polls closing, followed by a full export', async () => {
await withApp(
{},
async ({ apiClient, mockAuth, mockScanner, mockUsbDrive }) => {
async ({ apiClient, mockAuth, mockScanner, mockUsbDrive, workspace }) => {
await configureApp(apiClient, mockAuth, mockUsbDrive, { testMode: true });
await scanBallot(mockScanner, mockUsbDrive, apiClient, 0);
await scanBallot(mockScanner, mockUsbDrive, apiClient, 1);
await scanBallot(mockScanner, apiClient, workspace.store, 0);
await scanBallot(mockScanner, apiClient, workspace.store, 1);

expect(
await apiClient.exportCastVoteRecordsToUsbDrive({
Expand All @@ -239,9 +239,9 @@ test('continuous CVR export, including polls closing, followed by a full export'
test('CVR resync', async () => {
await withApp(
{},
async ({ apiClient, mockAuth, mockScanner, mockUsbDrive }) => {
async ({ apiClient, mockAuth, mockScanner, mockUsbDrive, workspace }) => {
await configureApp(apiClient, mockAuth, mockUsbDrive, { testMode: true });
await scanBallot(mockScanner, mockUsbDrive, apiClient, 0);
await scanBallot(mockScanner, apiClient, workspace.store, 0);

// When a CVR resync is required, the CVR resync modal appears on the "insert your ballot"
// screen, i.e. the screen displayed when no card is inserted
Expand Down Expand Up @@ -300,8 +300,8 @@ test('ballot batching', async () => {
}

// Scan two ballots, which should have the same batch
await scanBallot(mockScanner, mockUsbDrive, apiClient, 0);
await scanBallot(mockScanner, mockUsbDrive, apiClient, 1);
await scanBallot(mockScanner, apiClient, workspace.store, 0);
await scanBallot(mockScanner, apiClient, workspace.store, 1);
let batchIds = getBatchIds();
expect(getCvrIds()).toHaveLength(2);
expect(batchIds).toHaveLength(1);
Expand Down Expand Up @@ -338,8 +338,8 @@ test('ballot batching', async () => {
});

// Confirm there is a new, second batch distinct from the first
await scanBallot(mockScanner, mockUsbDrive, apiClient, 2);
await scanBallot(mockScanner, mockUsbDrive, apiClient, 3);
await scanBallot(mockScanner, apiClient, workspace.store, 2);
await scanBallot(mockScanner, apiClient, workspace.store, 3);
batchIds = getBatchIds();
expect(getCvrIds()).toHaveLength(4);
expect(batchIds).toHaveLength(2);
Expand Down Expand Up @@ -376,8 +376,8 @@ test('ballot batching', async () => {
});

// Confirm there is a third batch, distinct from the second
await scanBallot(mockScanner, mockUsbDrive, apiClient, 4);
await scanBallot(mockScanner, mockUsbDrive, apiClient, 5);
await scanBallot(mockScanner, apiClient, workspace.store, 4);
await scanBallot(mockScanner, apiClient, workspace.store, 5);
batchIds = getBatchIds();
expect(getCvrIds()).toHaveLength(6);
expect(batchIds).toHaveLength(3);
Expand Down
8 changes: 4 additions & 4 deletions apps/scan/backend/src/app_results.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,13 @@ beforeEach(() => {
test('end-to-end tabulated results', async () => {
await withApp(
{},
async ({ apiClient, mockScanner, mockUsbDrive, mockAuth }) => {
async ({ apiClient, mockScanner, mockUsbDrive, mockAuth, workspace }) => {
await configureApp(apiClient, mockAuth, mockUsbDrive, { testMode: true });

// scan a few ballots
await scanBallot(mockScanner, mockUsbDrive, apiClient, 0);
await scanBallot(mockScanner, mockUsbDrive, apiClient, 1);
await scanBallot(mockScanner, mockUsbDrive, apiClient, 2);
await scanBallot(mockScanner, apiClient, workspace.store, 0);
await scanBallot(mockScanner, apiClient, workspace.store, 1);
await scanBallot(mockScanner, apiClient, workspace.store, 2);

const allResults = await apiClient.getScannerResultsByParty();
expect(allResults).toHaveLength(1);
Expand Down
4 changes: 2 additions & 2 deletions apps/scan/backend/src/app_usb_drive.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ test('ejectUsbDrive', async () => {
test('doesUsbDriveRequireCastVoteRecordSync is properly populated', async () => {
await withApp(
{},
async ({ apiClient, mockAuth, mockUsbDrive, mockScanner }) => {
async ({ apiClient, mockAuth, mockUsbDrive, mockScanner, workspace }) => {
await configureApp(apiClient, mockAuth, mockUsbDrive, { testMode: true });
const mountedUsbDriveStatus = {
status: 'mounted',
Expand All @@ -57,7 +57,7 @@ test('doesUsbDriveRequireCastVoteRecordSync is properly populated', async () =>
mountedUsbDriveStatus
);

await scanBallot(mockScanner, mockUsbDrive, apiClient, 0);
await scanBallot(mockScanner, apiClient, workspace.store, 0);
await expect(apiClient.getUsbDriveStatus()).resolves.toEqual(
mountedUsbDriveStatus
);
Expand Down
8 changes: 8 additions & 0 deletions apps/scan/backend/src/scanners/custom/state_machine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -434,11 +434,15 @@ async function recordAcceptedSheet(
const { sheetId } = interpretation;
store.withTransaction(() => {
storeInterpretedSheet(store, sheetId, interpretation);

// If we're storing an accepted sheet that needed review, that means that it was "adjudicated"
// (i.e. the voter said to count it without changing anything).
if (interpretation.type === 'NeedsReviewSheet') {
store.adjudicateSheet(sheetId);
}

// Gets reset to false within exportCastVoteRecordsToUsbDrive
store.setIsContinuousExportOperationInProgress(true);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually implement this store method in the next commit

});
(
await exportCastVoteRecordsToUsbDrive(
Expand All @@ -460,10 +464,14 @@ async function recordRejectedSheet(
const { sheetId } = interpretation;
store.withTransaction(() => {
storeInterpretedSheet(store, sheetId, interpretation);

// We want to keep rejected ballots in the store, but not count them. We accomplish this by
// "deleting" them, which just marks them as deleted and is how we indicate that an interpreted
// ballot wasn't counted.
store.deleteSheet(sheetId);

// Gets reset to false within exportCastVoteRecordsToUsbDrive
store.setIsContinuousExportOperationInProgress(true);
});
(
await exportCastVoteRecordsToUsbDrive(
Expand Down
10 changes: 10 additions & 0 deletions apps/scan/backend/src/store.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -720,6 +720,16 @@ test('getBallotsCounted', () => {
expect(store.getBallotsCounted()).toEqual(1);
});

test('isContinuousExportOperationInProgress and setIsContinuousExportOperationInProgress', () => {
const store = Store.memoryStore();

expect(store.isContinuousExportOperationInProgress()).toEqual(false);
store.setIsContinuousExportOperationInProgress(true);
expect(store.isContinuousExportOperationInProgress()).toEqual(true);
store.setIsContinuousExportOperationInProgress(false);
expect(store.isContinuousExportOperationInProgress()).toEqual(false);
});

test('getExportDirectoryName and setExportDirectoryName', () => {
const store = Store.memoryStore();

Expand Down
45 changes: 41 additions & 4 deletions apps/scan/backend/src/store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,10 @@ export class Store {
private readonly uiStringsStore: UiStringsStore
) {}

// Used by shared CVR export logic in libs/backend
// eslint-disable-next-line vx/gts-no-public-class-fields
readonly scannerType = 'precinct';

getDbPath(): string {
return this.client.getDatabasePath();
}
Expand Down Expand Up @@ -823,15 +827,44 @@ export class Store {
return safeParseSystemSettings(result.data).unsafeUnwrap();
}

isContinuousExportOperationInProgress(): boolean {
const row = this.client.one(
`
select
is_continuous_export_operation_in_progress as isContinuousExportOperationInProgress
from is_continuous_export_operation_in_progress
`
) as { isContinuousExportOperationInProgress: number } | undefined;
return Boolean(row?.isContinuousExportOperationInProgress);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just added a type + basic utils to admin/backend/src/store.ts for being more explicit about these boolean conversions. What do you think about copying those or moving them to a shared library to use in this store file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nice! Can move to a shared location

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a9caa14

}

setIsContinuousExportOperationInProgress(
isContinuousExportOperationInProgress: boolean
): void {
this.client.run('delete from is_continuous_export_operation_in_progress');
this.client.run(
`
insert into is_continuous_export_operation_in_progress (
is_continuous_export_operation_in_progress
) values (?)
`,
isContinuousExportOperationInProgress ? 1 : 0
);
}

/**
* Gets the name of the directory that we're continuously exporting to, e.g.
* TEST__machine_SCAN-0001__2023-08-16_17-02-24. Returns undefined if not yet set.
*/
getExportDirectoryName(): string | undefined {
const result = this.client.one(
'select export_directory_name as exportDirectoryName from export_directory_name'
const row = this.client.one(
`
select
export_directory_name as exportDirectoryName
from export_directory_name
`
) as { exportDirectoryName: string } | undefined;
return result?.exportDirectoryName;
return row?.exportDirectoryName;
}

/**
Expand All @@ -840,7 +873,11 @@ export class Store {
setExportDirectoryName(exportDirectoryName: string): void {
this.client.run('delete from export_directory_name');
this.client.run(
'insert into export_directory_name (export_directory_name) values (?)',
`
insert into export_directory_name (
export_directory_name
) values (?)
`,
exportDirectoryName
);
}
Expand Down
7 changes: 4 additions & 3 deletions apps/scan/backend/test/helpers/custom_helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import {
waitForContinuousExportToUsbDrive,
waitForStatus,
} from './shared_helpers';
import { Store } from '../../src/store';

export async function withApp(
{
Expand Down Expand Up @@ -123,7 +124,7 @@ export async function withApp(
});
mockUsbDrive.assertComplete();
} finally {
await waitForContinuousExportToUsbDrive(mockUsbDrive);
await waitForContinuousExportToUsbDrive(workspace.store);
await new Promise<void>((resolve, reject) => {
server.close((error) => (error ? reject(error) : resolve()));
});
Expand Down Expand Up @@ -216,8 +217,8 @@ export function simulateScan(

export async function scanBallot(
mockScanner: jest.Mocked<CustomScanner>,
mockUsbDrive: MockUsbDrive,
apiClient: grout.Client<Api>,
store: Store,
initialBallotsCounted: number
): Promise<void> {
mockScanner.getStatus.mockResolvedValue(ok(mocks.MOCK_READY_TO_SCAN));
Expand All @@ -242,5 +243,5 @@ export async function scanBallot(
ballotsCounted: initialBallotsCounted + 1,
});

await waitForContinuousExportToUsbDrive(mockUsbDrive);
await waitForContinuousExportToUsbDrive(store);
}
25 changes: 8 additions & 17 deletions apps/scan/backend/test/helpers/shared_helpers.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
import { InsertedSmartCardAuthApi } from '@votingworks/auth';
import { ok } from '@votingworks/basics';
import {
areOrWereCastVoteRecordsBeingExportedToUsbDrive,
mockBallotPackageFileTree,
} from '@votingworks/backend';
import { mockBallotPackageFileTree } from '@votingworks/backend';
import { electionFamousNames2021Fixtures } from '@votingworks/fixtures';
import * as grout from '@votingworks/grout';
import {
Expand All @@ -24,6 +21,7 @@ import waitForExpect from 'wait-for-expect';
import { MockUsbDrive } from '@votingworks/usb-drive';
import { Api } from '../../src/app';
import { PrecinctScannerStatus } from '../../src/types';
import { Store } from '../../src/store';

export async function expectStatus(
apiClient: grout.Client<Api>,
Expand Down Expand Up @@ -99,18 +97,11 @@ export async function configureApp(
* while they're still being read from / written to.
*/
export async function waitForContinuousExportToUsbDrive(
mockUsbDrive: MockUsbDrive
store: Store
): Promise<void> {
// Check that mockUsbDrive.usbDrive.status has been configured before calling it
if (mockUsbDrive.usbDrive.status.hasExpectedCalls()) {
const usbDriveStatus = await mockUsbDrive.usbDrive.status();
await waitForExpect(
() =>
expect(
areOrWereCastVoteRecordsBeingExportedToUsbDrive(usbDriveStatus)
).toEqual(false),
10000,
250
);
}
await waitForExpect(
() => expect(store.isContinuousExportOperationInProgress()).toEqual(false),
10000,
250
);
}
10 changes: 1 addition & 9 deletions libs/backend/src/cast_vote_records/export.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,10 @@ import {
} from '../../test/utils';
import {
AcceptedSheet,
areOrWereCastVoteRecordsBeingExportedToUsbDrive,
clearDoesUsbDriveRequireCastVoteRecordSyncCachedResult,
doesUsbDriveRequireCastVoteRecordSync,
exportCastVoteRecordsToUsbDrive,
ExportOptions,
markCastVoteRecordExportAsInProgress,
RejectedSheet,
Sheet,
} from './export';
Expand Down Expand Up @@ -794,7 +792,7 @@ test.each<{
mockPrecinctScannerStore.setBallotsCounted(1);
const usbDriveStatus = await mockUsbDrive.usbDrive.status();
assert(usbDriveStatus.status === 'mounted');
await markCastVoteRecordExportAsInProgress(usbDriveStatus.mountPoint);
mockPrecinctScannerStore.setIsContinuousExportOperationInProgress(true);
},
shouldUsbDriveRequireCastVoteRecordSync: true,
},
Expand Down Expand Up @@ -1017,12 +1015,6 @@ test('cast vote record export clears doesUsbDriveRequireCastVoteRecordSync cache
).toEqual(false);
});

test('areOrWereCastVoteRecordsBeingExportedToUsbDrive returns false when no USB drive', () => {
expect(
areOrWereCastVoteRecordsBeingExportedToUsbDrive({ status: 'no_drive' })
).toEqual(false);
});

test('export and subsequent import of that export', async () => {
// Export
process.env['VX_MACHINE_TYPE'] = 'scan';
Expand Down
Loading